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(toggle-button): add disabled state #2202

Merged
merged 4 commits into from
Nov 20, 2023
Merged

Conversation

LuLaValva
Copy link
Member

@LuLaValva LuLaValva commented Nov 13, 2023

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

  • Add disabled state to toggle buttons

Screenshots

image image

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

@LuLaValva LuLaValva self-assigned this Nov 13, 2023
Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

First look through
Looks great
Only question: do we have a variable for opacity?

@LuLaValva
Copy link
Member Author

LuLaValva commented Nov 13, 2023

do we have a variable for opacity?

We do, but --opacity-50 is 0.04 so I'd need to make a new one called --opacity-625. I felt this was more descriptive.

@agliga
Copy link
Contributor

agliga commented Nov 13, 2023

do we have a variable for opacity?

We do, but --opacity-50 is 0.04 so I'd need to make a new one called --opacity-625. I felt this was more descriptive.

How does opaticty-50 set to 0.04 make any sense.
Is it defined in figma? Let's double check with Randy if needed

Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

Okay, I think the opacity is fine. We will get new tokens for that later.

That said, I think we should add aria-disabled as well like we have for button.
Other than that, LGTM

src/less/toggle-button/toggle-button.less Outdated Show resolved Hide resolved
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

The general approach of using opacity is probably the most preferable here. So that's good. Some things I found that we should address...

dist/toggle-button/toggle-button.css Outdated Show resolved Hide resolved
docs/_includes/toggle-button.html Outdated Show resolved Hide resolved
src/less/toggle-button/toggle-button.less Outdated Show resolved Hide resolved
src/less/toggle-button/toggle-button.less Show resolved Hide resolved
dist/toggle-button/toggle-button.css Outdated Show resolved Hide resolved
@ArtBlue
Copy link
Contributor

ArtBlue commented Nov 14, 2023

Let's also make sure to run Percy for toggle-button and toggle-button-group after all the issues are resolved and stories completed.

Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

I personally think its fine as is (after adding aria-disabled).
I think we can address other edge cases in a follow up or try to do them now.
I will approve on my end but Im fine to wait for Arthurs changes.

@LuLaValva
Copy link
Member Author

Thanks @agliga and @ArtBlue, I believe I addressed all of the comments

@LuLaValva LuLaValva requested a review from ArtBlue November 17, 2023 00:27
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Good work!

@LuLaValva LuLaValva merged commit 15e308e into 16.9.0 Nov 20, 2023
1 check passed
@LuLaValva LuLaValva deleted the disabled-toggle-button branch November 20, 2023 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants