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

fix: Right border of removable tag #650

Merged
merged 2 commits into from
Apr 5, 2019
Merged

Conversation

jonnybel
Copy link
Contributor

@jonnybel jonnybel commented Apr 5, 2019

Summary

Fixes #638

Kapture 2019-04-05 at 14 23 46
(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.

@jonnybel jonnybel added 🐛 Type: Bug Something isn't working Semver: FIX labels Apr 5, 2019
@jonnybel jonnybel requested review from montezume and emmenko April 5, 2019 12:40
@jonnybel jonnybel changed the title fix: right border of removable tag fix: Right border of removable tag Apr 5, 2019
@jonnybel
Copy link
Contributor Author

jonnybel commented Apr 5, 2019

I've added a fix to the focus styles for the [x] button to improve its a11y.

Copy link
Contributor

@montezume montezume left a 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,
Copy link
Contributor

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 {
Copy link
Contributor Author

@jonnybel jonnybel Apr 5, 2019

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.

Copy link
Member

@emmenko emmenko left a 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! 💯

@montezume montezume merged commit 425e82d into master Apr 5, 2019
@montezume montezume deleted the ja-fix-tag-right-border branch April 5, 2019 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants