-
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
feat(stepper): s2 number field/stepper migration #3681
Conversation
🦋 Changeset detectedLatest commit: bd77043 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 |
File metricsSummaryTotal size: 1.39 MB*
stepper
* 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.
e2354a0
to
b1f40f5
Compare
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
- 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
- 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)
- 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
- updates the calc for the input to stepper spacing to subtract the infield buttons' inline padding from the text-to-stepper padding - updates metadata
- add calc for validation icon to stepper spacing to subtract the infield button's inline padding
- correctly calculates the input padding to account for the spacing between the input and alert icon, alert icon size and the new spacing between the icon and the stepper buttons - updates metadata
- corrects the padding-inline-end for the textfield by adding the offset to the calc - removes the mod for textfield's min-width since it is being overridden by the numberfield's min-width 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.
f46d2d6
to
bd77043
Compare
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