restyle facet dropdown to match figma#409
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
There was a problem hiding this comment.
It looks good. Some comments (most small, but the biggest one, besides cross-component token usage, is about the possibility of a more generic select (with generic select tokens), that is worth discussion with Paulo, etc. Let me know what you think.
Another consideration is that when we have a single facet (and therefore an "inert" select box, it still looks like the select box should be clickable. This might be a discussion for design, but I think this should look and feel more "readonly" in this case.
| @@ -28,14 +28,15 @@ export const FacetSelector = ({ | |||
| if (!currentFacet || facets.length <= 1) { | |||
| return currentFacet ? ( | |||
| <div className={clsx(s.label, s.container)}> | |||
There was a problem hiding this comment.
If there is only one facet, this combines label and container together on the same container, and so the container gets the transparent background intended for the label (rather than the container).
There was a problem hiding this comment.
Ah very true, thanks!
| &:not([data-is-selected="true"]) { | ||
| @include hover { | ||
| background: rgb(var(--color-control-accent) / 6%); | ||
| background: rgb(var(--color-button-accent-subtle-base-hover)); |
There was a problem hiding this comment.
Here we are using the button token directly in another component (see the line in AGENTS.md).
| overflow: hidden; | ||
| background: rgb(var(--color-card-surface)); | ||
| border-radius: var(--border-radius-small); | ||
| background: rgb(var(--color-facet-selector-surface)); |
There was a problem hiding this comment.
While this component is a facet selector, it feels like the underlying component is a custom select box that could be used more generally in other contexts.
What do you think of converting this into a generic Select (with the styling applied to it), and then FacetSelector would be a wrapping component that uses Select internally?
There was a problem hiding this comment.
Totally agree, done!
|
|
||
| "facet-selector-surface": "$card-surface", | ||
| "facet-selector-border": [196, 199, 197], | ||
| "facet-selector-color": "$card-content", |
There was a problem hiding this comment.
The token here is aliasing another components tokens.
| width: 100%; | ||
| padding: 12px 16px; | ||
| color: rgb(var(--color-facet-selector-color)); | ||
| background: none; |
There was a problem hiding this comment.
This is worth double-checking (i.e. are these already reset this way upstream), but are these restates of already existing resets? (see the frontend style guide about restating the reset)
Thanks Nick, all feedback addressed! Totally agree on the select box, also sorry about all the token stuff, a little bit of a RTFM issue on my part 😳 I'll run the design question past Matt & see what he thinks. I agree it does seem like the single facet could use something to differentiate from the interactive dropdown |
|
I was not able to check on the facet new additions, but what I was hoping to have is the text centered with the facet info, and when there are many more facets to have a chevron (with the proper hover over behaviour and all that). |
a670c82 to
fa59581
Compare
fa59581 to
a3fbca5
Compare
3 participants
Overview