-
Notifications
You must be signed in to change notification settings - Fork 201
feat(stepper): s2 number field/stepper migration #3681
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
base: spectrum-two
Are you sure you want to change the base?
feat(stepper): s2 number field/stepper migration #3681
Conversation
🦋 Changeset detectedLatest commit: df46640 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
File metricsSummaryTotal size: 1.39 MB*
File change detailsstepper
toast
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
stylelint
Custom property --spectrum-number-field-edge-to-validation-icon-extra-large not defined
spectrum-css/components/stepper/index.css
Line 132 in 58fcc4f
--spectrum-numberfield-spacing-validation-icon-to-stepper-hidden: var(--spectrum-number-field-edge-to-validation-icon-extra-large); |
Expected empty line before rule
spectrum-css/components/stepper/index.css
Lines 246 to 250 in 58fcc4f
.spectrum-NumberField-textfield:not(:has(.spectrum-Textfield-validationIcon)) { | |
.spectrum-NumberField-input { | |
padding-inline-end: 0; | |
} | |
} |
Expected ".hide-stepper" to match pattern "^(spectrum-|is-|u-)[A-Za-z0-9-]+"
spectrum-css/components/stepper/index.css
Line 166 in 58fcc4f
--mod-textfield-min-width: var(--mod-numberfield-hidden-stepper-min-inline-size, var(--spectrum-numberfield-hidden-stepper-min-inline-size)); |
Expected ".hide-stepper" to match pattern "^(spectrum-|is-|u-)[A-Za-z0-9-]+"
spectrum-css/components/stepper/index.css
Line 231 in 58fcc4f
min-inline-size: var(--mod-numberfield-hidden-stepper-min-inline-size, var(--spectrum-numberfield-hidden-stepper-min-inline-size)); |
Expected ".hide-stepper" to match pattern "^(spectrum-|is-|u-)[A-Za-z0-9-]+"
spectrum-css/components/stepper/index.css
Line 240 in 58fcc4f
&.spectrum-Textfield.is-invalid .spectrum-Textfield-validationIcon { |
🚀 Deployed on https://pr-3681--spectrum-css.netlify.app |
55e94b1
to
840fffe
Compare
isInvalid, | ||
isFocused, | ||
isDisabled, | ||
displayLabel: false, |
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 hardcoded this displayLabel: false
(as opposed to displayLabel
getting passed into Textfield) so that we wouldn't get duplicate field labels.
Does that make sense, or should I refactor? I'm pretty sure I could just use the embedded field label (and maybe even the help text) that's already within the textfield component, with some additional CSS edits.
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.
Ohhh, this is a little weird... my initial instinct is that we shouldn't be dealing with field labels in each. But using textfield's field label also feels weird because it's a bit different (I think) from what SWC does, and it'd be better to be more aligned rather than less aligned.
It kind of feels like it's Textfield's fault that you have to do this. What would be pros/cons of separating the Textfield input from the Textfield with the field label and help text in the template? Like, that Textfield input could be a separate export that we could use in number field? I feel like Picker is also composed in this way? What do you think? If we did something like that, it doesn't have to be done here, I think this works fine for now, because if we change textfield's template, we'll start potentially affecting other components that use textfield too.
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.
Also, why is field label a boolean situation with label text and help text is just a string (in textfield)? That feels inconsistent.
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.
A few things to update here:
- I can now remove the
displayLabel: false
since this PR to address this was merged. - I don't mind the idea of separating the textfield input from the rest of textfield. That may encourage the "unstyled" input thing I tried in this PR. I'm happy to explore that some more.
- Right now, the
displayLabel
is a boolean for the field label because I'm pulling it from textfield's args.
Do you think I should refactor number field's field label arg to match label text and help text (getting rid of the boolean and just using a string?)
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 don't mind the idea of separating the textfield input from the rest of textfield. That may encourage the "unstyled" input thing I tried in this PR. I'm happy to explore that some more.
I wonder if that's worth exploring in more of an RFC, textfield ends up being a headache for SWC's number field too, would it potentially solve problems for them too if we had an unstyled input that we used?
Do you think I should refactor number field's field label arg to match label text and help text (getting rid of the boolean and just using a string?)
I'd be slightly in favor, but don't feel strongly!
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'll create a ticket to create an RFC I guess! I do think that would solve some of our problems, but I'd have to experiment more from the SWC side to verify that.
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.
@rise-erpelding Does this ticket (CSS-867) actually cover what we're talking about? It has some browser support steps to it, but sounds like some research into styling the inputs is involved.
...customClasses.reduce((a, c) => ({ ...a, [c]: true }), {}), | ||
})} | ||
@keyup=${function(e) { |
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.
Inspiration from this PR: #3354
I wanted to make sure the correct keyboard focus styles were being applied when I actually navigated to the number field with the tab key.
e2354a0
to
b1f40f5
Compare
@@ -46,7 +46,7 @@ | |||
display: block; | |||
} | |||
|
|||
/* Fix extra space after inline-flex elements such as stepper. */ | |||
/* Fix extra space after inline-flex elements such as number field. */ |
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.
Feel free to tell me to revert this- it's probably not necessary but I wanted to clarify the component's new name.
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.
Ohh I'd put it on my to-do list to look at why this component had changes. From a release standpoint, it seems ok? Once it's migrated, it'll be bumped to the next major version anyway.
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.
The only things I changed in form's CSS is this comment (and then the form stories, I had to update the import to NumberField
).
Is the comment change necessary right now- probably not. I was just trying to cover all the instances of the word "stepper" and update the language to "number field."
Think I should remove this comment?
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'm not upset if we leave it, but here's another thought:
The only change we're making is to change stepper to number field, right?
If we decide to change the package name down the road (to change stepper.stories.js, stepper.test.js, components/stepper, etc.), maybe we can roll that change in there? Maybe if it's a ticket, the AC can be to ensure that there are no more instances of stepper anywhere in the codebase?
components/stepper/index.css
Outdated
--spectrum-numberfield-background-color-disabled: var(--mod-numberfield-background-color-disabled, var(--spectrum-disabled-background-color)); | ||
--spectrum-numberfield-background-color-disabled: var(--mod-numberfield-background-color-disabled, var(--spectrum-gray-25)); |
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 have 2 background-color-disabled props currently. The first matched the token specs, where disabled-background-color
is used. The second follows what's actually defined in Figma, which is gray-25
.
gray-25
looks like all of the Figma designs, where using disabled-background-color
(even though that's what's in the token specs) looks more like S1. I think @rise-erpelding had the same issue in the textfield S2 migration.
isInline: true, | ||
size, | ||
customClasses: [`${rootClass}-button`], | ||
isDisabled, |
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 just added a click to the inline buttons while I was playing around with this. You can now decrement or increment. I was thinking not add some interactivity for fun?
onAdd: function () {
const newValue = String(parseFloat(value) + 0.5);
updateArgs?.({ value: newValue });
},
onMinus: function () {
const newValue = String(parseFloat(value) - 0.5);
updateArgs?.({ value: newValue });
},
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.
Oh! I didn't realize we already had these args baked into the the inline buttons so we could pass down increment/decrement functions. We often try to avoid JavaScript implementation in the template because it's unnecessary overhead and we don't ship it anywhere, but this is a really small enhancement that I think would be nice.
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'll see if I can work these in. 👍
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.
@rise-erpelding @aramos-adobe Here's the work on the infield button JS!
Awesome work on this @marissahuysentruyt ! Only thing I can think of right now is adding the Storybook downstate interaction on the infield button and click functions in the isInline clicks to add and subtract. |
32555b2
to
785d2f9
Compare
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.
This is looking great considering it's a really huge migration (from what it looked like before)! Left a few small comments, and I'm wondering particularly about the textfield passthroughs on the CSS.
isInvalid, | ||
isFocused, | ||
isDisabled, | ||
displayLabel: false, |
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.
Also, why is field label a boolean situation with label text and help text is just a string (in textfield)? That feels inconsistent.
components/stepper/index.css
Outdated
--mod-textfield-border-color-invalid-keyboard-focus: var(--mod-numberfield-border-color-invalid-keyboard-focus, var(--spectrum-numberfield-border-color-invalid-keyboard-focus)); | ||
--mod-textfield-icon-spacing-block-invalid: calc(var(--spectrum-numberfield-spacing-block-start-edge-to-alert-icon) - var(--spectrum-numberfield-border-width)); | ||
--mod-textfield-icon-spacing-inline-end-invalid: var(--spectrum-numberfield-spacing-validation-icon-to-stepper); | ||
--mod-textfield-width: var(--mod-numberfield-inline-size, var(--spectrum-numberfield-inline-size)); |
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.
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 would be all for removing any of the mods for textfield if we can! What's the risk if I don't set mods- if the stepper variables don't load, would the styles be wrecked?
In this PR, I tried to emulate React's number field a bit more, and they do more of a "hidden" or "unstyled" input. So there was a little bit different approach. I will be honest that I didn't go through the mods initially, so I'll go through them again and see if I can remove/refactor some.
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.
The idea of building on something that's more unstyled rather than our existing, styled, textfield input is really interesting. My instinct is to leverage mods for now wherever possible over setting new styles, but I wonder if that's misguided? It feels like it might be less code overall to use mods, but it's possible we haven't set textfield up in the best way for numberfield to use it? For instance, I see in this screenshot that .spectrum-NumberField-textfield
is set up to use display: inline-flex
which is overriding .spectrum-Textfield
that has display: inline-grid
.
But in any case, refining a bit to ensure that we're not setting both a mod and then also a style that overrides the mod would be ideal, and if there's a situation where a mod just can't do the job, make sure we have that explained in a comment?
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.
That's what I tried to do in this commit: 107d9a9
I went through each mod, and if the mod did the trick I removed the duplicate style definition. If it didn't do the trick, I removed the mod and left a comment in the styles about what it's doing.
Let me know what you think.
.spectrum-NumberField-input { | ||
line-height: var(--spectrum-numberfield-line-height); | ||
padding-inline-start: 0; | ||
padding-inline-end: var(--spectrum-numberfield-spacing-min-inline-end-text-to-stepper); |
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'm wondering if the spacing needs a few adjustments, I noticed that when I type in many numbers, the padding creates a lot of white space between the last number and the buttons, definitely more than the spec:
Inspecting it seems like that text-to-stepper token is being overridden so it never gets used:
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.
Fantastic catch- thank you! I think just removing that padding-inline-end you mentioned fixed 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.
Looking at the token spec "Spacing (minimum, text to stepper)", I'm thinking that this spacing needs to be from the edge of the input to the edge of the first button, but the buttons already have padding, so this is more padding overall than what we need.
I think there are multiple ways to deal with this, but one way is that we can make this padding a calculation that subtracts the spacing from the buttons (I didn't look at the specs for the infield button, so I'm not sure if that calc would ever result in a negative number... if that's the case we have to look at using margin or something else instead).
isInline: true, | ||
size, | ||
customClasses: [`${rootClass}-button`], | ||
isDisabled, |
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.
Oh! I didn't realize we already had these args baked into the the inline buttons so we could pass down increment/decrement functions. We often try to avoid JavaScript implementation in the template because it's unnecessary overhead and we don't ship it anywhere, but this is a really small enhancement that I think would be nice.
- updates rootClass to NumberField - imports new nested components - use inline infield buttons instead of separate stacked infield buttons - adds support for side label - updates templates for docs page
- updates documentation to use number field language instead of stepper - imports new numberfield templates - adds support for displaying the field label, label position, help text and label text - adds side label, invalid states stories - updates some sentence-case display names for stories - removes quiet variant stories - updates display title to Number field
- updates any stepper language to number field - adds test coverage for hidden stepper, side label, help text, invalid, minimum width, and truncation - adds test case for focus + hover, keyboardFocus + hover, disabled+ keyboardFocus
- new tokens used - cleans up/standardizes mod usage for textfield - cleans up/standardizes selectors so they are consistent across states/ variants - updates all custom properties to use numberfield prefix instead of stepper - adds style definitions for new features including hidden stepper, side label, help text, invalid, minimum width
- updates form to use number field import
- removes i18n test comments - removes duplicate disabled+keyboard-focused test
785d2f9
to
c183290
Compare
- uses textfield mods where possible to reduce the number of custom styles number field has - removes any textfield mods that are overwritten by number field styles - updates metadata - general clean up (stylelint warnings are addressed, some corrected mod definitions)
c183290
to
107d9a9
Compare
- adds styles for read-only state - adds test cases - adds story for read-only state on docs page and expands on some documentation - updates metadata
7e8269c
to
4ce8d19
Compare
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.
Sorry this took longer than I thought! It took a little bit to understand some of the issues with spacing that I pointed out here, and I'm definitely not confident that I unearthed all of them.
I made some pretty minor suggestions aside from spacing, you can take or leave those depending on what you think. It's mainly the spacing that's having me hit "Request changes" now.
I think you mentioned the idea of having an unstyled text input that we could use in textfield, text area, number field, and combobox? After walking through some of the complexity here and realizing that we have to override textfield's styles in order to get the infield buttons in here, I really like that idea.
Let me know if you want to walk through any of the feedback or troubleshoot anything together!
* Number fields have a [field label](/docs/components-field-label--docs) that is positioned above the field by default, with a | ||
* secondary option to be positioned on the side of the field. Having the label on the top is the default and is recommended | ||
* because this works better with long copy, localization, and responsive layouts. The [inline infield buttons](/docs/components-in-field-button--docs#inline) |
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.
These parts about the number field label would also fit nicely above the side label story, but they're also ok here. Up to you.
/** | ||
* Labels can be placed either on top or on the side of the field. Having the label on the side is most useful for when vertical space is limited. | ||
*/ | ||
export const SideLabel = AllDefaultVariantsGroup.bind({}); |
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.
This could use a SideLabel.storyName
to control capitalization more, if you want.
text: helpText, | ||
variant: isInvalid ? "negative" : undefined, | ||
hideIcon: true, | ||
isDisabled: isDisabled || isReadOnly, |
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 find it a little unexpected that read-only number field has disabled help text and buttons, but we don't have a whole lot of guidance for this, right? It's ok with me to keep it help text and buttons disabled for read-only for 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.
Correct, no real guidance on it. I followed React's read-only treatment for the buttons, and frankly, I wasn't sure what to do with the help text.
There's a follow up ticket for this: CSS-33
* are usually visible, but can be hidden. The amount of the increment/decrement step is 1 by default. | ||
* | ||
* When the label is too long for the available space, it will wrap to the next line. If the value within the number field input | ||
* overflows, it will truncate with an ellipsis. |
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.
A few other notes I found in the S2 / Desktop spec that I thought were interesting that you might consider optionally including:
- To view truncated text, a tooltip can be made available on hover (desktop) or long press (mobile). We don't have to implement that here, but we could note it.
- If an error state is needed for smaller inputs, consider hiding the error icon so that more space of the value to be visible.
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.
@@ -46,7 +46,7 @@ | |||
display: block; | |||
} | |||
|
|||
/* Fix extra space after inline-flex elements such as stepper. */ | |||
/* Fix extra space after inline-flex elements such as number field. */ |
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'm not upset if we leave it, but here's another thought:
The only change we're making is to change stepper to number field, right?
If we decide to change the package name down the road (to change stepper.stories.js, stepper.test.js, components/stepper, etc.), maybe we can roll that change in there? Maybe if it's a ticket, the AC can be to ensure that there are no more instances of stepper anywhere in the codebase?
&::before { | ||
content: ""; |
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.
Is this ::before
doing something I'm not understanding?
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.
That is a fabulous question. I don't know what it is. I wanted to remove it, but saw that React has a blank before
too, so I chickened out. Their's is also hidden and 0px wide. 🤷♀️ The only thing I thought of was maybe it's to accommodate a leading icon, sort of like picker does. But I don't see that support in Figma anywhere.

https://react-spectrum.adobe.com/s2/index.html?path=/docs/numberfield--docs
/* gives the textfield container (instead of the input) the proper border styles */ | ||
border: var(--spectrum-numberfield-border-width) solid var(--spectrum-numberfield-border-color-default); | ||
padding-inline-start: var(--spectrum-numberfield-spacing-inline-edge-to-text); | ||
padding-inline-end: var(--spectrum-numberfield-button-container-size); |
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 so if I use the inspector to hover this .spectrum-NumberField-textfield
and hover over .spectrum-NumberField-buttons
, I'm eyeballing it but it looks like there might be more space on the textfield than just what the buttons take up?
The left side of this padding looks like it might be just a little bit more than what we need, and I'm wondering if it's because we need to subtract border width here?
If I hover over .spectrum-NumberField-buttons
for this particular size (it's small in this screenshot) it looks like it's set to start 0px from the edge, which I believe is correct. But we're putting padding on .spectrum-NumberField-textfield
, which is also where we've put the border, so the padding is all inside of the border rather than on the border which is where .spectrum-NumberField-buttons
aligns, and so I think that might be why the spacing looks a little larger than what we need here?
.spectrum-NumberField-input { | ||
line-height: var(--spectrum-numberfield-line-height); | ||
padding-inline-start: 0; | ||
padding-inline-end: var(--spectrum-numberfield-spacing-min-inline-end-text-to-stepper); |
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.
Looking at the token spec "Spacing (minimum, text to stepper)", I'm thinking that this spacing needs to be from the edge of the input to the edge of the first button, but the buttons already have padding, so this is more padding overall than what we need.
I think there are multiple ways to deal with this, but one way is that we can make this padding a calculation that subtracts the spacing from the buttons (I didn't look at the specs for the infield button, so I'm not sure if that calc would ever result in a negative number... if that's the case we have to look at using margin or something else instead).
--mod-textfield-spacing-block-start: var(--mod-numberfield-spacing-block-start-edge-to-text, var(--spectrum-numberfield-spacing-block-start-edge-to-text)); | ||
--mod-textfield-spacing-block-end: var(--mod-numberfield-spacing-block-end-edge-to-text, var(--spectrum-numberfield-spacing-block-end-edge-to-text)); | ||
--mod-textfield-icon-spacing-block-invalid: calc(var(--spectrum-numberfield-spacing-block-start-edge-to-alert-icon) - var(--spectrum-numberfield-border-width)); | ||
--mod-textfield-icon-spacing-inline-end-invalid: var(--spectrum-numberfield-spacing-validation-icon-to-stepper); |
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.
Should this one be in the next section where you talk about inline-size overrides?
I'm wondering though if it would just be a better idea to override the style instead of using the --mod
for inline spacing, there's a lot more complexity here than I initially realized.
These --mod-textfield-icon-spacing-inline-end-invalid
and --mod-textfield-icon-spacing-inline-start-invalid
mods for instance will determine the amount of space we give between the end of the input (where we can actually put numbers) and the start of the icon, right? But using these mods mean that we use textfield's padding-inline-end calculation for invalid, which subtracts the border width, but since we have infield buttons in the way most of the time, do we still need to account for border width in the calculation? I'm questioning it because it's hard to say for me that we're definitely 2px off when it looks more or less ok, what do you think?
|
||
/* this mod overrides the inline-size of the nested textfield */ | ||
--mod-textfield-width: 100%; | ||
--mod-textfield-height: calc(var(--mod-numberfield-block-size, var(--spectrum-numberfield-block-size)) - var(--mod-numberfield-border-width, var(--spectrum-numberfield-border-width)) * 2); |
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.
If we decide not to use mods for inline spacing, I think this height, width, and all the other mods that don't relate to spacing at all are still fair game to be utilized here. Really nice work sorting that out, I didn't see any overridden styles that also had crossed out mods from numberfield this time around.
Description
Welcome to the new & improved number field (previously known as stepper)! 🥳🔢 This migration was quite complex, and added a LOT of new features to our humble number field.
New features include:
Following React's lead, the markup of the number field has changed. You'll see that the
.spectrum-Textfield
div is where the S2 design language lives (instead of on the input element), while the actualinput
is unstyled and incorporated more subtly. Based on design's, React's and SWC's naming conventions, the display name for this component has also changed fromstepper
tonumberField
. This introduced breaking changes in all previous custom properties, where any--spectrum-stepper-*
or--mod-stepper-*
properties moved to--spectrum-numberfield-*
or--mod-numberfield-*
. This also applied to class names, where.spectrum-Stepper
changed to.spectrum-NumberField
. Thehide-stepper
class has also been updated to match our class naming conventions (.spectrum-NumberField--hiddenStepper
).Because of all the new features and to align more with design, SWC and React, below is a quick recap of the following tokens & classes that have been renamed in the CSS:
.spectrum-Stepper*
class names have been converted to.spectrum-NumberField*
.hide-stepper
class has been converted to.spectrum-NumberField--hiddenStepper
--spectrum-stepper*
to--spectrum-numberfield*
--mod-stepper*
to--mod-numberfield
Removed custom properties include:
--mod-stepper-animation-duration
--mod-stepper-button-border-width
--mod-stepper-button-width
--mod-stepper-button-width-quiet
--mod-stepper-buttons-background-color
--mod-stepper-buttons-border-color
--mod-stepper-buttons-border-color-focus
--mod-stepper-buttons-border-color-focus-hover
--mod-stepper-buttons-border-color-hover
--mod-stepper-buttons-border-color-keyboard-focus
--mod-stepper-buttons-border-style
--mod-stepper-buttons-border-width
--mod-stepper-focus-indicator-visibility
--mod-stepper-height (renamed to
--mod-numberfield-block-size
)--mod-stepper-width (renamed to
--mod-numberfield-inline-size
)New custom properties include:
--mod-numberfield-background-color
--mod-numberfield-background-color-disabled
--mod-numberfield-block-size (renamed from
--mod-stepper-height
)--mod-numberfield-border-color-disabled
--mod-numberfield-border-color-invalid-default
--mod-numberfield-border-color-invalid-focus
--mod-numberfield-border-color-invalid-focus-hover
--mod-numberfield-border-color-invalid-hover
--mod-numberfield-border-color-invalid-keyboard-focus
--mod-numberfield-button-inline-offset
--mod-numberfield-font-family
--mod-numberfield-font-size
--mod-numberfield-font-style
--mod-numberfield-font-weight
--mod-numberfield-hidden-stepper-min-inline-size
--mod-numberfield-inline-size (renamed from
--mod-stepper-width
)--mod-numberfield-label-to-field
--mod-numberfield-line-height
--mod-numberfield-min-inline-size
--mod-numberfield-spacing-block-end-edge-to-text
--mod-numberfield-spacing-block-start-edge-to-text
--mod-numberfield-spacing-field-to-helptext
This PR also incorporates the refactoring and test coverage that was introduced and tested in both #3558 and #3670 (both of which could be closed after this PR is merged 👍)
Jira/Specs
CSS-1120
Figma token specs (under Number field)
Figma desktop designs (under Number field)
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
field-label-to-component
(inspect.spectrum-NumberField-fieldLabel
)in-field-stepper-to-end-*
sizing tokens to correspond to t-shirt sized number fields (inspect.spectrum-NumberField-buttons
)help-text-to-component
(inspect.spectrum-NumberField-helpText
in an invalid number field, for instance)field-default-width-*
sizing tokens to correspond to t-shirt sized number fieldsnumber-field-minimum-width-multiplier
(inspect.spectrum-NumberField--hiddenStepper
/the hidden stepper number field)number-field-with-stepper-minimum-width-*
sizing tokens to correspond to t-shirt sized number fields (inspect.spectrum-NumberField
)component-height-*
sizing tokens to correspond to t-shirt sized number fields (inspect.spectrum-NumberField-textfield
)corner-radius-medium-size-*
sizing tokens to correspond to t-shirt sized number fields (inspect.spectrum-NumberField-textfield
)component-edge-to-text-*
sizing tokens to correspond to t-shirt sized number fields (inspect.spectrum-NumberField-textfield
)component-top-to-text-*
sizing tokens to correspond to t-shirt sized number fields (inspect.spectrum-NumberField-textfield
)component-bottom-to-text-*
sizing tokens to correspond to t-shirt sized number fields (inspect the textfield mods in.spectrum-NumberField-input.spectrum-Textfield-input
)text-to-visual-*
sizing tokens to correspond to t-shirt sized number fields (inspect the.spectrum-NumberField-input
element of a default number field)border-width-200
(inspect.spectrum-NumberField-textfield
)focus-indicator-thickness
,focus-indicator-color
, andfocus-indicator-gap
(inspect.spectrum-NumberField-inputs
in a keyboard focused number field)workflow-icon-size-*
for each t-shirt size (for invalid number fields)component-top-to-workflow-icon-*
for each t-shirt size (check through the mods for textfield's icon spacing)text-to-visual-*
for each t-shirt size (inspect.spectrum-NumberField-input.spectrum-Textfield-input
in the invalid number field)number-field-visual-to-in-field-stepper-*
for each t-shirt size (inspect.spectrum-NumberField-input.spectrum-Textfield-input
in the invalid number field)gray-25
(inspect.spectrum-NumberField-textfield
and the mods for textfield's background color)gray-300
,gray-400
,gray-800
andgray-900
for the various default variant border colors and states (inspect.spectrum-NumberField-textfield
)negative-border-color-default
,negative-border-color-hover
,negative-border-color-focus-hover
,negative-border-color-focus
andnegative-border-color-key-focus
for the various invalid variant border colors and states (inspect.spectrum-NumberField-textfield
)neutral-content-color-default
,neutral-content-color-hover
,neutral-content-color-focus-hover
,neutral-content-color-focus
andneutral-content-color-key-focus
for the various id variant border colors and states (inspect.spectrum-NumberField-input
and mods for the textfield text color)negative-visual-color
of the icondisabled-content-color
,disabled-border-color
,disabled-background-color
for the disabled statedefault-font-family
,regular-font-weight
,default-font-style
,line-height-100
as font stylescjk-line-height-100
as font styles in the CJK contextsfont-size-*
for each t-shirt sizeAdditional Validation
Stepper
in your IDE. No function calls, and no imports in references toStepper
should exist. As a caveat, there will still be references to stepper. The NPM package is still named "stepper," and all of the previous changelogs still use "stepper" as the component name, so not everything has been converted to "number field." We also have arguments in stories file that use the word "stepper," but all of the function calls to this component and any imports should be changed toNumberField
.hide-stepper
in your IDE. Nothing should be returned (except maybe some older JSON).--spectrum-Stepper
and--mod-stepper
in your IDE. Nothing should be returned except for changelogs.Regression testing
Validate:
Screenshots
S1


S2


To-do list