feat(clearbutton): s2 migration#4043
Conversation
🦋 Changeset detectedLatest commit: c3950f9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
68af820 to
6459841
Compare
File metricsSummaryTotal size: 1.43 MB*
clearbutton
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
📚 Branch previewPR #4043 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-4043/index.html. |
6459841 to
3e1e802
Compare
rise-erpelding
left a comment
There was a problem hiding this comment.
It's nice to have this one pared down a little for S2!
It looks good, I have a question about the down state. Looking at the token specs and also digging around the component playground/Explore component behavior section in Figma, it looks like this clear button receives the S2 down state treatment where the button gets a tiny bit smaller to give the appearance of being pushed in. Unless there's a good reason it's not included, we probably need to add it. There's some documentation in our Storybook already with examples on how it's used in components if you haven't added one of these in a component migration before. I believe the notes there came from design at some point.
Figma always gives the same 3 down state tokens for every component where it's used, but you probably don't need all of them. The documentation will explain that for 24px or less, you can use the component-size-minimum-perspective-down token rather than the component-size-width-ratio-down (using the ratio requires you to have the height/width of the component, which you'll need the withDownStateDimensionCapture decorator for)... since the clear button is larger than 24px but the icons are all smaller, it seems like it might be ok to use the component-size-minimum-perspective-down token?
A couple of other thoughts related to down state here:
- We'll probably want to capture this in the tests, as well as the hover state
- I'm wondering how this will look in the removable Tag, which uses the clear button. The down state for Tag applies the down state to the whole Tag, but I'm guessing that if we were only targeting the clear button within Tag, that ideally we'd want just the clear button to have the down state and not the whole tag? That might be another ticket to review and confirm with design later on.
cde74ff to
bdf9f5b
Compare
We do have the color tokens in place for each state, but that predates the migration. ✨
That makes the most sense to me — I've gone ahead and run with
Added tests for hover and down. ✨
Hmmm, that's a good question. Looking at the removable tag now, the down state applied to the whole tag doesn't feel quite right. I'd be inclined to apply it to the child clear button like you suggest. I'll open up a ticket. |
6d9fb56 to
f3224fb
Compare
f3224fb to
306ab28
Compare
marissahuysentruyt
left a comment
There was a problem hiding this comment.
Nice work! There's some very small things I think we should add, like the migrated status & tag, and then the active and hover controls. I think the tokens are in a good but, I had a couple of extra things I wanted to bring up!
Do we know if the corner radius needs to be anything specific for clear button? Should it be fully rounded into a circle, or follow the infield button border radius specs? I very well may have missed it if you already asked this question somewhere! I noticed there were no rounded corners at all when I was playing with the mod for the background color, that's all.
I noticed that we get the browser default focus-visible outline on the clear button. But looking at the infield button CSS (which this is sort of a variant of), the outline is removed completely. Do you think we should also remove the outline for keyboard focus in clear button?
marissahuysentruyt
left a comment
There was a problem hiding this comment.
Nice work! I left a final note about the border radius for future us (in case we hear that the border radius needs to be changed back to a circle or be something else).
|
|
||
| inline-size: 100%; | ||
| block-size: 100%; | ||
| border-radius: 100%; |
There was a problem hiding this comment.
I don't want to block this from merging, but just wanted to note once more about the border radius here. You may have asked somewhere that I just missed. It could be a real quick follow up.
Do we know if the corner radius needs to be anything specific for clear button? Should it be fully rounded into a circle as it was previously in S1, or does it follow the infield button border radius specs? Those were the only tokens I don't see, and it's a little unclear to me just looking all over in Figma.
3f4f65c to
c3950f9
Compare
|
|
||
| #### S2 migration for clear button | ||
|
|
||
| This migrates the clear button component to the latest Spectrum 2 designs. Custom properties have been remapped and added per the design specification. |
There was a problem hiding this comment.
Were any mod tokens changed or added that we should note in the changelog?
| "@spectrum-css/clearbutton": major | ||
| --- | ||
|
|
||
| #### S2 migration for clear button |
There was a problem hiding this comment.
| #### S2 migration for clear button | |
| #### S2 migration |
Minor knit but this will get added to the clear button component's changelog so the context will ultimately be clear from that.
|
|
||
| This migrates the clear button component to the latest Spectrum 2 designs. Custom properties have been remapped and added per the design specification. | ||
|
|
||
| - The quiet variant has been remove as all clear buttons have a transparent background. |
There was a problem hiding this comment.
| - The quiet variant has been remove as all clear buttons have a transparent background. | |
| - The `quiet` variant has been **removed** as all clear buttons have a transparent background now, making this variant irrelevant. |
Minor formatting and wording knit.
| This migrates the clear button component to the latest Spectrum 2 designs. Custom properties have been remapped and added per the design specification. | ||
|
|
||
| - The quiet variant has been remove as all clear buttons have a transparent background. | ||
| - The over-background and static color variants have been removed. |
There was a problem hiding this comment.
| - The over-background and static color variants have been removed. | |
| - The `over-background` and `static color` variants have been removed. |
What does this translate to in the code? I like this general note but might be good to translate this into, X class was removed for example.
|
|
||
| - The quiet variant has been remove as all clear buttons have a transparent background. | ||
| - The over-background and static color variants have been removed. | ||
| - Test and stories for deprecated variants have been removed. |
There was a problem hiding this comment.
I like this note but it doesn't really impact customers so maybe we remove this point?
| - The quiet variant has been remove as all clear buttons have a transparent background. | ||
| - The over-background and static color variants have been removed. | ||
| - Test and stories for deprecated variants have been removed. | ||
| - The down state has been added, corresponding controls and tests have been added. |
There was a problem hiding this comment.
What does that translate to for customers? Can you clarify in the docs here what the introduction of the down state looks like for customers adopting this change?
| border-radius: 100%; | ||
|
|
||
| background-color: var(--mod-clear-button-background-color, transparent); | ||
| background-color: transparent; |
There was a problem hiding this comment.
Nice, let's make sure this gets documented clearly in the changelog that this mod no longer works here.
Description
CSS-1169This migrates the clear button component to the latest Spectrum 2 designs. Custom properties have been remapped and added per the design specification.
To-do list