-
Notifications
You must be signed in to change notification settings - Fork 26
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: Right border of removable tag #650
Conversation
I've added a fix to the focus styles for the [x] button to improve its a11y. |
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 good to me 👍
@@ -247,7 +266,8 @@ const Tag = props => ( | |||
background: inherit; | |||
border-style: solid; | |||
border-width: 1px 1px 1px 1px; | |||
&:hover { | |||
&:hover, |
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.
👍
@@ -283,10 +284,6 @@ const Tag = props => ( | |||
`, | |||
props.isDisabled && | |||
css` | |||
&:hover { |
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.
This isn't needed because when a button is disabled in the HTML, the hover
and focus
states are not triggered.
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 good to me, nice solution! 💯
Summary
Fixes #638
(GIF compression messing those colours 🧐)
Description
Using a regular
border
doesn't fit the design because the border takes space and would add a little edge or a flicker between the tag and the removal button.The easiest alternative would be using
box-shadow
to emulate the border, just on the right side, but when the tag has an onClick prop, it has a box-shadow of its own when hovered, so this new box-shadow border would have to be merged with the current box-shadow and added to the design tokens.I didn't like where this was going so I tried another approach:
So I used an absolutely positioned
:after
pseudo-element, and it does the job.