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

fix(alert): apply styling to disabled items #18545

Merged
merged 15 commits into from Aug 20, 2019

Conversation

kelvindart
Copy link
Contributor

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?

ion-select-option[disbled] have no reflective styling, so visibly do not look any different to non-disabled options.

Issue Number: #18542

What is the new behavior?

  • Disabled options now have an opacity of 0.3.
  • This is applied via targeting [disabled] .alert-inner-button

Does this introduce a breaking change?

  • Yes
  • No

Other information

Without styling:
ion-select-option no styling

With styling:
ion-select-option no styling


[disabled] .alert-button-inner {
cursor: default;
opacity: .3;

Choose a reason for hiding this comment

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

Please use opacity: .5; as it is the style commonly used for other disabled buttons.

Choose a reason for hiding this comment

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

Done 🙂

@@ -133,6 +133,16 @@
</ion-select>
</ion-item>

<ion-item>
Copy link
Contributor

Choose a reason for hiding this comment

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

The Statuses alert select looks like it already has a disabled button in its test, so not sure if this test gives us any additional coverage.

@kelvindart-certua
Copy link

Hey @liamdebeasi - good point. I think I added it to test both the attribute and the property. Shall I remove it?

@liamdebeasi
Copy link
Contributor

Yeah probably best to remove it. At this point, I think that would just be testing Stencil's functionality.

@kelvindart-certua
Copy link

kelvindart-certua commented Jul 17, 2019

Awesome - I shall do. Thanks! :) I'll ping you once I've done it.

@kelvindart
Copy link
Contributor Author

Hey @liamdebeasi - I've reverted the commit. Hopefully ready to merge now 🙂.

@liamdebeasi
Copy link
Contributor

Thanks! We actually use classes to indicate disabled states to make it easier for users to customize, so I've updated your PR with that. Other than that, I think we're good to go!

@kelvindart-certua
Copy link

Hey @liamdebeasi - I saw this was merged a few days ago, was it included in 4.7.0? No problem if not, I'm just checking :). If it wasn't, any idea which version it will appear in?

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Jul 26, 2019

@kelvindart-certua This has not been merged in yet. 4.7.0 had a fix for disabled items in ion-datetime. We hope to have this PR merged in soon, thanks!

@kelvindart-certua
Copy link

kelvindart-certua commented Jul 26, 2019

No worries - just seen master was merged into it (I misread it initially and thought it was merged into master, which is why I was surprised it wasn't in 4.7.0). No problem either way - thanks 😁.

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.

Nice job!

@liamdebeasi liamdebeasi merged commit 67ed89d into ionic-team:master Aug 20, 2019
@liamdebeasi
Copy link
Contributor

Merged. Thank you!

brandyscarney pushed a commit that referenced this pull request Aug 27, 2019
* fix(datetime): ensure hasValue returns correct value

* fix(alert): apply styling to disabled items

* test(datetime): add tests for empty value datetime

* fix(): update getter for stencil update

* fix: change default opacity to disabled button opacity

* test(select): add tests to ion-select-option disabled items

* Revert "test(select): add tests to ion-select-option disabled items"

This reverts commit b7ac867

* update disabled css to use classes
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.

None yet

6 participants