-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: Create New Tag Theme #10100
Feat: Create New Tag Theme #10100
Conversation
✅ ethereum-org-website-dev deploy preview ready
|
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.
Nicely done @TylerAPfledderer. Left a few comments but overall this looks great.
}, | ||
label: { | ||
// ...warningLabelStyles | ||
}, |
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 think we also need to style the closeButton
s pseudo classes with the same styles that the container has. I'm talking specifically about the focus state.
Right now we are displaying the default Chakra styles here. I think we should use the same border styles and colors that we use for the container.
This is how it looks now when you focus on the close button:
Not saying that this is the code but could be something like:
closeButton: {
_focusVisible: {
...statusStyles["&:any-link"]._focusWithin,
bg: "transparent",
boxShadow: "none",
},
},
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.
@pettinarip This example you gave will actually work here.
I think what I am missing here is whether or not this button will actually be visible if the tag itself is a link. That might change how I approach this to ensure the focus ring for the button has enough contrast to the background of the tag (looking at the normal
tag)
@nloureiro I don't believe we received that clarity in the DS yet. 😄
Will the close button ever be used if the tag is a link?
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.
it's a corner case, we might never have that.
For me, Tags must be prepared to be links, normally in the form of filters.
The close on the Tag is when you have a filter list and you want to remove that filter. (we currently don´t have this on ethereum.org)
if it's complex to have link + close has a possible variant I'm ok with don't adding this now and update when needed
does it help?
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.
@nloureiro Oh! I understand.
I think it will be best to have three stories here:
- Show all tag style variants as links with text and no icons to simply demonstrate the state changes (maybe they should include the + icon as the right-side element?).
- Provide another set of the same tags, but not as a links and containing the close button to demonstrate the button's styling in the context of the different style variants
- The different variants of elements that are expected to be rendered in the component.
Would this be better?
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.
hum... I think it might work, as far as I understand.
correct me if I'm wrong:
we will have a folder in storybook with "tags" > inside there will be 3 stories
is that it?
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.
Correct @nloureiro here you will see another entry
@TylerAPfledderer yes, make 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.
Sweet! I'll get that added asap
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.
look good to me, great job!!
@nloureiro @pettinarip update pushed with a third story added and found a cheap solution to get hover/focus styling for the close button. 🚨 Also for Pablo: need to update branch to |
// Clear default | ||
_focusVisible: null, | ||
"&:focus-visible, &:hover": { | ||
outline: "3px solid", | ||
outlineOffset: "-2px", | ||
}, |
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.
Cheap solution to get focus/hover styling for the close button.
- override default styling by nulling out (because I'm using a multi-selector instead)
- Provide a more generous styling of an outline ring on hover or focus-visible (this is taking advantage of the default
outlineColor
value ofcurrentColor
)
Anything else preferred here?
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.
Nice! looks great now
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.
@pettinarip I think a larger line height than |
Is there any URL where I can check this? |
got it, thank you @pettinarip @TylerAPfledderer one detail I missed in the Design system is the hover for the close button. with the border only is weird. so I'm adding this behavior to the Figma but my idea is to go full background like this screenshot what to you think? Screen.Recording.2023-05-18.at.15.43.06.mov |
@nloureiro this can be done, but will add a little complexity to our large object of styles.
Screenshot for subtle variant alternative when hovered (I still need to figure out how to refactor this huge object of styles. I would come up with some easy iterator, but there are one or two styles that do not line up with the rest to take advantage of the semantic token names. This will be a WIP past this PR unless I can think of something soon!) Also, I was unaware that there were modifications to the color palette! 😅 (The |
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.
Ok, I think we are in a good state to merge this, great job again @TylerAPfledderer and @nloureiro for the review.
A few things I've taken note of and that we are going to fix in different PRs:
- Close button styles: Tyler is looking into how we can simplify the styling object structure to include these changes with ease [Tag] Update close button styles #10266
- Fix multi-line tag height [Tag] Fix multi-line height #10265
Description
This is a part of implementing V1 of the New Design System
This PR modifies the existing
Tag
component and updates it's theme to match the new DS.Also update the filter tags in the Developer Tutorials list page to ensure they do not receive the new styling in prod for the moment.