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

commented Jun 14, 2019

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

liamdebeasi added 3 commits Jun 14, 2019

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

This comment has been minimized.

Copy link
@pranjaljately

pranjaljately Jul 4, 2019

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

This comment has been minimized.

Copy link
@kelvindart-certua
@@ -133,6 +133,16 @@
</ion-select>
</ion-item>

<ion-item>

This comment has been minimized.

Copy link
@liamdebeasi

liamdebeasi Jul 17, 2019

Member

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

This comment has been minimized.

Copy link

commented Jul 17, 2019

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

@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

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

@kelvindart-certua

This comment has been minimized.

Copy link

commented Jul 17, 2019

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

@kelvindart

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

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

liamdebeasi added 3 commits Jul 23, 2019
@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

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!

liamdebeasi and others added 3 commits Jul 23, 2019
@kelvindart-certua

This comment has been minimized.

Copy link

commented Jul 26, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

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 😁.

@liamdebeasi
Copy link
Member

left a comment

Nice job!

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

1 check passed

build Workflow: build
Details
@liamdebeasi

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Merged. Thank you!

brandyscarney added a commit that referenced this pull request Aug 27, 2019
fix(alert): apply styling to disabled items (#18545)
* 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
Projects
None yet
6 participants
You can’t perform that action at this time.