-
Notifications
You must be signed in to change notification settings - Fork 841
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
Add disabled state to EuiBadge #2440
Conversation
src-docs/src/views/badge/badge.js
Outdated
]; | ||
|
||
export default () => ( | ||
<EuiFlexGroup wrap responsive={false} gutterSize="xs" style={{ width: 300 }}> | ||
{badges.map(badge => ( | ||
<EuiFlexItem grow={false} key={badge}> | ||
<EuiBadge color={badge}>{badge}</EuiBadge> | ||
<EuiBadge isDisabled={badge === '#fea27f' ? true : false} color={badge}> |
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.
Looks like all your docs examples did make it into the PR. I think it's good to have one example, but the others can probably be reverted. I would just make "disabled" another option in the list above.
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.
Looks like you're still making the custom colored one disabled. Can you instead, append to the list a color specifically called 'disabled'
?
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.
Just a couple more items but definitely like the button style. Also, we'll need a test for this new prop as well.
src-docs/src/views/badge/badge.js
Outdated
]; | ||
|
||
export default () => ( | ||
<EuiFlexGroup wrap responsive={false} gutterSize="xs" style={{ width: 300 }}> | ||
{badges.map(badge => ( | ||
<EuiFlexItem grow={false} key={badge}> | ||
<EuiBadge color={badge}>{badge}</EuiBadge> | ||
<EuiBadge isDisabled={badge === '#fea27f' ? true : false} color={badge}> |
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.
Looks like you're still making the custom colored one disabled. Can you instead, append to the list a color specifically called 'disabled'
?
src/components/badge/notification_badge/_notification_badge.scss
Outdated
Show resolved
Hide resolved
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.
Great, lgtm! I just had one comment about adding a comment, but then it's good to merge.
@@ -56,6 +56,8 @@ export type EuiBadgeProps = { | |||
*/ | |||
color?: IconColor; | |||
|
|||
isDisabled?: boolean; |
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.
Since we're also overriding the colors passed in here, it might be nice to add a docs comment that expresses that behavior.
Summary
Added disabled state to
EuiBadge
. I also added some examples to the docs. This is to cover #2355For the case of badges with onClick events here is the behavior when they're disabled.
Note: the disabled examples shown on the gif above are not in the docs but I can include them if we think they're needed.
Checklist
- [ ] Checked in mobile- [ ] Props have proper autodocs- [ ] Added or updated jest tests- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes