feat(infield-button): new s2 migration #3642
Conversation
🦋 Changeset detectedLatest commit: 52079e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
aramos-adobe
added
size-3 M
aramos-adobe
force-pushed
the
aramos-adobe/css770-infield-button-s2-migration
branch
from
12677f3 to
7776a89
Compare
File metricsSummaryTotal size: 1.38 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
File change detailsinfieldbutton
tokens
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
|
🚀 Deployed on https://pr-3642--spectrum-css.netlify.app |
|
Running through the token changes/updates now and I don't see the I didn't see Or are those specific to parent components? ✨ |
I added the |
There was a problem hiding this comment.
My first pass is everything but the CSS. I think there's some room for improvement for the supported icons. We'd have to check with design, I wouldn't think every UI icon is acceptable to use in the infield button. I really liked the idea of showcasing the available icons, but since "Add" isn't a UI icon, we may have to tweak that control some. It honestly might be easier to just list out the 4 icons that seem to be available for this button (according to the design specs).
If we can figure it out, I'm kind of digging the table for "Supported icons." (don't mind the placeholder circle where the add/plus should be! 😆 )
| if: false, | ||
| description: "All UI icons have sizes of `s`, `m`, `l`, and `xl` except for `ArrowDown`, `ArrowLeft`, `ArrowRight`, and `ArrowUp` which only have sizes of `m`. `Asterisk` has all sizes except for `s`.", |
There was a problem hiding this comment.
This is a really great description- very helpful when I was looking at it trying to figure out what it was for! I do, however, have some suggestions for it.
- My first question is- are all of the available UI icons supported in the infield button? Like, am I allowed or would design be ok with consumers using just an asterisk or just the corner triangle in this component? It feels like maybe not, but I could be wrong. Currently, I can get a button that is "icon-less" at some sizes.
- The UI icons don't actually come in sizes like small, medium and large- they have the associate numbers like 100, 200, 300. I think we should clarify the description in some way to not get the t-shirt sizes mixed up with the size index number. ("index" is how that number is referenced in the tokens repo)
- So the plus sign isn't a UI icon, it's a workflow. With the controls for the stacked story disabled, how can we demonstrate that setup? Does this just have to be a set list of the icon options?
- With all of that...should that description change? Maybe we can supplement this description on the docs page. What do you think about doing a table on the docs page for the supported icons like clear button has? It could even be the docs for a "supported icons" story.
There was a problem hiding this comment.
@aramos-adobe I think most of the questions I had in this ☝️ comment were answered in the slack thread, but just wanted point it out once more. Based on Kayiu's comment about "those are the 4 specific use cases that we have" and that she was "not sure if the design library would offer all the icons as choices in the future," I think we need to rework this iconName control.
Happy to pair when you're back!
There was a problem hiding this comment.
Ok, here's my CSS review! Things are looking good overall- I know we've talked in slack now that design only supports the 4 UI icons, and that CSS is missing the "add" UI icon. I made a branch that experiments with supporting only the 4 icons (I had to come up with a temporary fix to work around not having the add UI icon), and then worked a little on the CSS (doing the custom variables for the missing tokens).
I figured a branch was more beneficial than me trying to explain it all via comments!
components/infieldbutton/index.css
Outdated
| } | ||
| } | ||
|
|
||
| @media (forced-colors: active) { | ||
| .spectrum-InfieldButton-stacked { |
There was a problem hiding this comment.
Is there a particular reason for this selector to be first? I wonder if we reorganized the file to put this variant after the default .spectrum-InfieldButton selector that we could depend on the cascade more, and we wouldn't have to redeclare a bunch of the t-shirt size style definitions.
It just seems repetitive to have padding-inline: var(--mod-infield-button-side-edge-to-fill, var(--spectrum-infield-button-side-edge-to-fill)); and then the t-shirt sizes with the same padding-inline: var(--mod-infield-button-side-edge-to-fill, var(--spectrum-infield-button-side-edge-to-fill));
components/infieldbutton/index.css
Outdated
| --highcontrast-infield-button-border-color: ButtonText; | ||
| --highcontrast-infield-button-border-color-active: Highlight; | ||
| padding-inline: var(--mod-infield-button-side-edge-to-fill, var(--spectrum-infield-button-side-edge-to-fill)); | ||
| inline-size: calc(var(--spectrum-infield-button-width) - 6px); |
There was a problem hiding this comment.
I think the hardcoded pixel values in these calcs represent the missing token values, correct? It doesn't look like in-field-button-side-edge-to-fill is defined in our token set yet. 😫.
I tested some of the reorganizing I mentioned in the comment above, and I can actually remove quite a bit of the styles, including this inline-size with the hardcoded 6px and 8px. (
// I moved the default .spectrum-InfieldButton styles to line 104.
.spectrum-InfieldButton {
border-style: none;
background-color: transparent;
cursor: pointer;
display: flex;
justify-content: center;
align-items: center;
block-size: var(--mod-infield-button-height, var(--spectrum-infield-button-height));
inline-size: var(--mod-infield-button-width, var(--spectrum-infield-button-width));
padding: var(--mod-infield-button-edge-to-fill, var(--spectrum-infield-button-edge-to-fill));
&:disabled {
cursor: auto;
}
&:focus-visible {
outline: none;
}
}
// I moved the `-paired` styles below that ☝️ , and then I could remove all of the extra t-shirt sized code.
.spectrum-InfieldButton-paired {
display: flex;
// I added the & to scope these styles specifically to the -paired class
& .spectrum-InfieldButton {
// I kept the redefinitions you had, but used the custom props instead.
inline-size: calc(var(--mod-infield-button-width, var(--spectrum-infield-button-width)) - var(--mod-infield-button-width, var(--spectrum-infield-button-edge-to-fill)));
padding-inline: var(--mod-infield-button-side-edge-to-fill, var(--spectrum-infield-button-side-edge-to-fill));
}
& > .spectrum-InfieldButton.spectrum-InfieldButton--sizeS {
inline-size: var(--mod-infield-button-width, var(--spectrum-infield-button-width));
}
}
// and then the rest of your CSS is the same...
.spectrum-InfieldButton-fill {
block-size: 100%;
inline-size: 100%;
...
There was a problem hiding this comment.
Nice work on this! I love the new table for the supported icons! There's a few things I think we can iron out, so we'll have to coordinate when you're back!
I wasn't sure where to put this, but wanted to get your opinion. What do you think we should do about the plus/add icon? I left a few comments about it, just so I didn't forget.
Currently, that "plus" sign icon in the inline variant is actually the "add" workflow icon, and the classes in the table are the workflow icon classes. We know that we're missing the add icon from the UI icon set right now (as opposed to the workflow icon set). Should we just update all of the add icons to be the add icon from UI? That would leave us with blank buttons for some time though, and we'd have to update the classes in the table, but...does that seem reasonable to do? I'd love to chat about it more and hear your opinions on it. I'm not sure, this is all just stuff I'm wondering about 🤔
.changeset/true-shrimps-dream.md
Outdated
|
|
||
| ### In-field button S2 Migration | ||
|
|
||
| In-field buttons are used to represent actions within input fields. They’re currently used inside the combo box, number field, and search field. This component has updated colors, corner radius, and icons compared to the Spectrum 1 version. The “Error” and “Key-focus” variants have been removed, since this now utilizes those states from the parent component. |
There was a problem hiding this comment.
I think we should clarify that only number field uses this component. Search field uses a clear button and combobox is using the picker button.
There was a problem hiding this comment.
@marissahuysentruyt, Picker button doesn't exist in Spectrum and design uses infield button as a nested component within combobox. We could extend the infield button into picker button though.
There was a problem hiding this comment.
@aramos-adobe Hmmm, yeah I see what you're saying. You're right that "picker button" (or clear button, for that matter) isn't the terminology that design uses.
The changelogs are probably geared towards developers, so to me, speaking in code terms would be clearer. I was imagining that if someone went into the combobox code, they'd only see the "picker button," not an "infield button." Also, if people are looking at our storybook (which has both components) or SWC's storybook (which also has both), there might be some confusion. I didn't immediately associate combobox and search with infield buttons, since those nested components aren't our infield button.
Maybe this is fine for the time being (it is the spectrum-two branch, so it's not released quite yet), so I'll defer to you. You and I have already talked about the naming between our repo and design, so I think this is another argument for opening that conversation with the larger team. We should add it to a sync soon!
tokens/custom/large-vars.css
Outdated
| --spectrum-in-field-button-side-edge-to-fill-small: 4px; | ||
| --spectrum-in-field-button-side-edge-to-fill-medium: 3px; | ||
| --spectrum-in-field-button-side-edge-to-fill-large: 4px; | ||
| --spectrum-in-field-button-side-edge-to-fill-extra-large: 4px; |
There was a problem hiding this comment.
I think we have some of the values mixed up here from in Figma. For large/mobile, we should have:
--spectrum-in-field-button-side-edge-to-fill-small: 3px;
--spectrum-in-field-button-side-edge-to-fill-medium: 4px;
--spectrum-in-field-button-side-edge-to-fill-large: 4px;
--spectrum-in-field-button-side-edge-to-fill-extra-large: 4px;
Unless you know differently, it looks like the values for small & medium need to switch.
| if: false, | ||
| description: "All UI icons have sizes of `s`, `m`, `l`, and `xl` except for `ArrowDown`, `ArrowLeft`, `ArrowRight`, and `ArrowUp` which only have sizes of `m`. `Asterisk` has all sizes except for `s`.", |
There was a problem hiding this comment.
@aramos-adobe I think most of the questions I had in this ☝️ comment were answered in the slack thread, but just wanted point it out once more. Based on Kayiu's comment about "those are the 4 specific use cases that we have" and that she was "not sure if the design library would offer all the icons as choices in the future," I think we need to rework this iconName control.
Happy to pair when you're back!
tokens/custom/medium-vars.css
Outdated
|
|
||
| --spectrum-in-field-button-side-edge-to-fill-small: 3px; | ||
| --spectrum-in-field-button-side-edge-to-fill-medium: 4px; | ||
| --spectrum-in-field-button-side-edge-to-fill-large: 4px; | ||
| --spectrum-in-field-button-side-edge-to-fill-extra-large: 4px; |
There was a problem hiding this comment.
| --spectrum-in-field-button-side-edge-to-fill-small: 3px; | |
| --spectrum-in-field-button-side-edge-to-fill-medium: 4px; | |
| --spectrum-in-field-button-side-edge-to-fill-large: 4px; | |
| --spectrum-in-field-button-side-edge-to-fill-extra-large: 4px; | |
| --spectrum-in-field-button-side-edge-to-fill-small: 4px; | |
| --spectrum-in-field-button-side-edge-to-fill-medium: 3px; | |
| --spectrum-in-field-button-side-edge-to-fill-large: 4px; | |
| --spectrum-in-field-button-side-edge-to-fill-extra-large: 4px; |
These tokens are listed differently in Figma. I think they must have changed where both sets are the same. But these are supposed to be different from the values in large-vars.css, correct?
|
|
||
| /** | ||
| * The in-field button component is a button used inside a text field. | ||
| * In-field buttons are used to represent actions within input fields. They’re currently used inside the [number field,](/docs/components-stepper--docs). |
There was a problem hiding this comment.
Just a typo with a stray comma after "number field"
| isInline: { | ||
| name: "Side by side", | ||
| type: { name: "boolean" }, |
There was a problem hiding this comment.
I'll leave this up to you, but some of our other components have control descriptions, so we could add a description here too. It could just be simple, like "Renders two infield buttons to be used as a numeric stepper" or something even more general.
Just a thought, not a blocker!
| }, | ||
| { | ||
| testHeading: "Stacked", | ||
| testHeading: "Side by side", | ||
| isStacked: true, |
There was a problem hiding this comment.
Oh, this isStacked got changed to isInline.
There was a problem hiding this comment.
This is really close to an approval from me! I know you're working on a few more things, but thanks for all your hard work on it! I do have some parting thoughts for you:
-
Do we need the
component-size-minimum-perspective-downtoken? I don't think I saw it used in the CSS, but I'm still fuzzy on when specifically we need that. Would design have called that out in the token specs, or should we clarify? It feels like maybe just for the small size, sort of like action button only uses that token for the 2 smallest sizes? You might know better than me, but here's what I'm referring to: -
We can't validate the add icon until we get the literal icon added to our package. Just wanted to note that here. I'll double check to see if we have a ticket for it (I think Josh created one)
-
Also wanted to note that we have a discussion set up for the terminology our code uses, vs what's in Figma and how design refers to some of these components. I think both you and I would definitely like to see our code align more with what design is doing. I think it would make things loads easier if everyone was using the same language. This PR in particular really showed those inconsistencies!
| chromatic: { disableSnapshot: true }, | ||
| }; | ||
|
|
||
| export const Quiet = Template.bind({}); | ||
| /** | ||
| * The quiet variant is used when the in-field button needs to be less visually prominent since it is used in input fields. This is typically used with the cross icon in search fields to clear the entered value. |
There was a problem hiding this comment.
Since we removed the language about search technically not having the infield button in the changeset, we should probably update this description of the quiet variant to remove the last sentence.
| --spectrum-infield-button-icon-color-hover: var(--spectrum-neutral-content-color-hover); | ||
| --spectrum-infield-button-icon-color-down: var(--spectrum-neutral-content-color-down); | ||
| --spectrum-infield-button-icon-color-key-focus: var(--spectrum-neutral-content-color-key-focus); | ||
| /* TODO: using custom in-field-button-side-edge-to-fill-* while token is unavailable */ |
There was a problem hiding this comment.
Great comment here!
components/infieldbutton/index.css
Outdated
| --spectrum-infield-button-background-color: var(--mod-infield-button-background-color-quiet, transparent); | ||
| --mod-infield-button-background-color-hover: var(--mod-infield-button-background-color-hover-quiet, transparent); | ||
| --mod-infield-button-background-color-down: var(--mod-infield-button-background-color-down-quiet, transparent); | ||
| --mod-infield-button-background-color-key-focus: var(--mod-infield-button-background-color-key-focus-quiet, transparent); |
There was a problem hiding this comment.
This change from a mod to a spectrum variable is interesting. Should this background color token get reverted back to the mod, since the rest of this stack is --mod variables? It's got a more specific mod in the definition, that consumers can use to do their customizations, so I think it'd be ok if it stayed a mod. It just stuck out since it didn't follow the pattern of the other blocks using mods.
It makes me think of the the rest of the mods in this file- do we need them? That's a larger refactoring decision and discussion, but we did just talk about mods today, so it's on my mind. 😆
components/infieldbutton/index.css
Outdated
| &:not(:disabled):focus-visible { | ||
| --highcontrast-infield-button-border-color: var(--highcontrast-infield-button-border-color-active); | ||
| } | ||
| transform: perspective(var(--spectrum-infield-button-downstate-perspective)) translateZ(var(--spectrum-component-size-difference-down)); |
There was a problem hiding this comment.
Again, not technically blocking, just a nitpick to be consistent: could we move this transform into the style definitions below, instead of with the token definitions up here? I think it should be a simple swap:
// @ line 111
.spectrum-InfieldButton {
... // keep the border-style and all the stuff that's already here
&:active {
transform: perspective(var(--spectrum-infield-button-downstate-perspective)) translateZ(var(--spectrum-component-size-difference-down));
}
}
There was a problem hiding this comment.
This looks really good! Just a few questions from me for now:
I'm not seeing the "Add" icon showing up when running locally or in the PR preview, but looking at previous screenshots of this PR it looks like it is supposed to be appearing, and shouldn't be blank. Is Add a workflow icon? Is that why it's disappeared?
I've also noticed the down state behaving irregularly. When I first load the story, it works, but for instance if I navigate to the testing preview, then back to the default story view, it stops working. Does that happen for you also? I have been having the same issue with Tag, so it's not just you, it may be a problem with the down state decorator. Not a blocker as far as I'm concerned, we can get that sorted out later.
@rise-erpelding, So yes, in the screenshots you see, I initially added the workflow icon to the inline variant. There are some missing icons in the ui iconSet including the Add one. We brought this to the attention of the design team here. So we thought when the icons eventually get updated the infield button would automatically show the Add icon in this variant. For the downstate action, I didn't notice it until you mentioned it. But I'm glad I'm not the only experiencing it 😁 Should we add this a ticket so we can investigate this bug? |
…ps://github.com/adobe/spectrum-css into aramos-adobe/css770-infield-button-s2-migration
aramos-adobe
deleted the
aramos-adobe/css770-infield-button-s2-migration
branch
Description
In-field buttons are used to represent actions within input fields. They’re currently used in the number field. This component has updated colors, corner radius, and icons compared to the Spectrum 1 version. The “Error” and “Key-focus” variants have been removed, since this now utilizes those states from the parent component.
The position styles and controls have also been removed now that there's a consistent corner radius.
How and where has this been tested?
Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
Open the Infield button story:
Regression testing
Validate:
Screenshots
To-do list