-
Notifications
You must be signed in to change notification settings - Fork 67
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
Conversation
There was a problem hiding this 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?
We do, but |
How does opaticty-50 set to 0.04 make any sense. |
There was a problem hiding this 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
There was a problem hiding this 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...
Let's also make sure to run Percy for |
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work!
Description
Screenshots
Checklist