-
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(listbox-button): added error state #2244
Conversation
.listbox-button button.btn--form:active { | ||
.listbox-button:not(.listbox-button--error) button.btn--form:hover, | ||
.listbox-button:not(.listbox-button--error) button.btn--form:focus, | ||
.listbox-button:not(.listbox-button--error) button.btn--form:active { |
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.
Hmm, Im wondering. Do we need the :not
?
The way I see it, if we do not have the error class then we should show the default color. If the error class is present, that should be a stronger style match and it should take precedence, right?
.listbox-button button.btn--form:hover { }
.listbox-button.listbox-button--error button.btn--form:hover { }
That should be stronger right?
Just wondering so we can simplify things.
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.
Yea, I changed the default to exclude error because it seemed odd that a focused hover/active/focus would remove the state indication; it would remove an important visual cue that there's an error on the listbox.
I suppose we can add another style block to override the default, but that would mean more code and explicit style application. I actually don't see how these error states were intended to work with hover/active/focus to begin with; I don't see anything in Figma. I'll reach out to DS as they'll probably need to provide the colors to use in this states.
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, lets leave it. I see what the issue is, we don't have a hover state for error. If we did that means we dont need this.
Lets add a comment to those lines to remove them when we get an error state. Otherwise 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.
I realized that having a focus state that didn't change color was likely an a11y
issue so I just reverted this and made the styling match textbox. We can apply the additional colors if/when we get them.
Fixes #2063
Description
This adds error state support for listbox-button as well as its broader support in
field
with an associated error message.Notes
This is the first phase of support for error state. The next issue/PR will add it for textbox and include the error message icon as well. The future message module will later extend both.
We can't run visual regression at the moment since Percy's on the fritz.
Screenshots
Listbox Button Before
Listbox Button After
Field Listbox Button After
Checklist