Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(alert): alert input custom attributes #21365

Merged
merged 23 commits into from
Jun 1, 2020

Conversation

cerkiner
Copy link
Contributor

@cerkiner cerkiner commented May 24, 2020

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

No custom attributes can be added for the inputs in the alert controller. Maxlength, pattern, class etc. attributes can be greatly useful when added to the inputs in the alert controller. I think we should be able to add such features.

Issue Number: resolves #21202, #21290, #7819

What is the new behavior?

This change introduces 'cssClass' and 'attributes' properties for inputs.

let alert = this.alertCtrl.create({
    inputs: [
      {
        ...
        cssClass: 'anyClass',
        attributes: { 
                inputmode: 'decimal',
                minlength: 1,
                maxlength: 4,
                autocomplete: 'off',
                ...
        }
        ...
      }
    ]
  });
  alert.present();
}

Does this introduce a breaking change?

  • Yes
  • No

Other information

@ionitron-bot ionitron-bot bot added the package: core @ionic/core package label May 24, 2020
@cerkiner
Copy link
Contributor Author

cerkiner commented May 24, 2020

npm run build process in core directory upgraded slides/swiper/swiper.bundle.js.
I have committed this change separately in 3e9919c

I am not sure if it belongs here.

Edit; test-core-clean-build fails because of this file.

@cerkiner cerkiner closed this May 24, 2020
@cerkiner cerkiner reopened this May 24, 2020
@cerkiner cerkiner force-pushed the alert-input-attributes branch 2 times, most recently from 8bc2c79 to 9454e55 Compare May 25, 2020 17:20
@cerkiner
Copy link
Contributor Author

cerkiner commented May 25, 2020

I rebased all to current master to resolve the issue. I removed that commit and updated the branch. Now, all checks have passed and the change looks fine.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! We are discussing how we want to document this, but the functionality works well.

core/src/components/alert/alert.tsx Outdated Show resolved Hide resolved
cerkiner added 5 commits May 27, 2020 14:41
onInput is essential for passing value changes on dissmiss
Warning: The "value" prop of <input> should be set after "min", "max", "type" and "step"
'alert-input' and  'alert-input-disabled' classes are important for consistent ionic style.
Overriding disabled with attributes.disabled causes mixed state and classes. This change fixes the issue.
@cerkiner
Copy link
Contributor Author

cerkiner commented May 27, 2020

The latest change lets the user completely override all attributes except onInput, class and disabled. The reason behind this is overriding these attributes can cause unexpected issues.

  • In the case of overriding onInput, values are not synced properly. Fixing this was essential for passing value changes on dismiss.

  • In the case of overriding class, input losses ionic designs. Fixing this was important to provide a better look.

  • In the case of overriding disabled, dom disabled state and ionic disabled classes were mixed. Fixing this was important to reflect a better look and functionality.

Users are still able to set these attributes but they are not overridden completely. They are merged for an optimum solution, please check and comment on the end result.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! A few more small things I noticed.

core/src/components/alert/test/translucent/index.html Outdated Show resolved Hide resolved
core/src/components/alert/test/standalone/index.html Outdated Show resolved Hide resolved
core/src/components/alert/alert.tsx Outdated Show resolved Hide resolved
@cerkiner
Copy link
Contributor Author

  • Unnecessary tests are removed. (I copied the same input in all tests only because I didn't fully understand the purposes of those.)

  • About disabled property, I got your point, I made a change to ensure higher specificity always wins.

{
  disabled: true,
  attributes: {
    disabled: false
  }
}

Results in false.

  • I merged the latest master to the branch, resolved the conflicts.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last comment then I think we're good to go 😄

core/src/components/alert/alert.tsx Outdated Show resolved Hide resolved
@cerkiner
Copy link
Contributor Author

resolved the conflicts for the latest master branch 👍

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: AlertController inputs should be having minlength, maxlength options for text fields
2 participants