Skip to content

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

Open
wants to merge 17 commits into
base: spectrum-two
Choose a base branch
from

Conversation

marissahuysentruyt
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt commented Apr 21, 2025

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:

  • an error state with alert icon
  • optional help text
  • embedded field label & optional positions

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 actual input 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 from stepper to numberField. 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. The hide-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:

  • All .spectrum-Stepper* class names have been converted to .spectrum-NumberField*
  • The .hide-stepper class has been converted to .spectrum-NumberField--hiddenStepper
  • Custom properties have been renamed from --spectrum-stepper* to --spectrum-numberfield*
  • Modifiable custom properties have been renamed from --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

  • Pull down the branch to run this branch locally or visit the deploy preview.
  • Navigate to the number field docs page.
  • Verify that the new documentation is grammatically correct according to Spectrum content guidelines, and makes sense/is understandable.
  • Verify new links to other components' documentation work as expected.
  • Navigate to the default story page.
  • Using the Storybook controls, create different variations for the component including more complex combinations:
    • focus
    • focus + hover
    • disabled + invalid
    • invalid + keyboard focus
    • disabled + focus
    • side label only
    • side label + help text
    • invalid + help text
    • light vs dark mode
    • medium/desktop vs. large/mobile
    • and any other combinations you can create and test!
  • Following the token specs, verify number field's usage of the following tokens:
    • 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 fields
    • number-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, and focus-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 and gray-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 and negative-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 and neutral-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 icon
    • disabled-content-color, disabled-border-color, disabled-background-color for the disabled state
    • default-font-family, regular-font-weight, default-font-style, line-height-100 as font styles
    • cjk-line-height-100 as font styles in the CJK contexts
    • font-size-* for each t-shirt size
  • Visit the Chromatic test preview.
  • Verify that the tests look as expected and have covered a large variety of test cases for the number field.

Additional Validation

  • Navigate to the forms docs page.
  • If you haven't already, pull down the branch to search the repo.
  • Search for Stepper in your IDE. No function calls, and no imports in references to Stepper 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 to NumberField.
  • Search for hide-stepper in your IDE. Nothing should be returned (except maybe some older JSON).
  • Search for --spectrum-Stepper and --mod-stepper in your IDE. Nothing should be returned except for changelogs.
  • Feel free to view the Chromatic test preview in WHCM to verify the component aligns with textfield (maybe it's not identical, but it should align closely).

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

S1
Screenshot 2025-04-22 at 8 31 37 AM
Screenshot 2025-04-22 at 9 09 13 AM

S2
Screenshot 2025-04-22 at 9 06 33 AM
Screenshot 2025-04-22 at 9 09 33 AM

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@marissahuysentruyt marissahuysentruyt self-assigned this Apr 21, 2025
Copy link

changeset-bot bot commented Apr 21, 2025

🦋 Changeset detected

Latest commit: df46640

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@spectrum-css/stepper Major
@spectrum-css/form Patch

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

Copy link
Contributor

github-actions bot commented Apr 21, 2025

File metrics

Summary

Total size: 1.39 MB*
No change in file sizes

Package Size Minified Gzipped
stepper 27.41 KB 26.35 KB 2.97 KB
toast 7.86 KB 7.48 KB 1.55 KB

File change details

stepper

Filename Head Minified Gzipped Compared to base
index.css 27.41 KB 26.35 KB 2.97 KB 🔴 ⬆ 9.12 KB
metadata.json 13.83 KB - - 🔴 ⬆ 6.00 KB

toast

Filename Head Minified Gzipped Compared to base
index.css 7.86 KB 7.48 KB 1.55 KB 🔴 ⬆ 1.31 KB
metadata.json 4.09 KB - - 🔴 ⬆ 0.31 KB
* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

Copy link
Contributor

@github-actions github-actions bot left a 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

⚠️ [stylelint] <spectrum-tools/no-unknown-custom-properties> reported by reviewdog 🐶
Custom property --spectrum-number-field-edge-to-validation-icon-extra-large not defined

--spectrum-numberfield-spacing-validation-icon-to-stepper-hidden: var(--spectrum-number-field-edge-to-validation-icon-extra-large);


⚠️ [stylelint] <rule-empty-line-before> reported by reviewdog 🐶
Expected empty line before rule

.spectrum-NumberField-textfield:not(:has(.spectrum-Textfield-validationIcon)) {
.spectrum-NumberField-input {
padding-inline-end: 0;
}
}


⚠️ [stylelint] <selector-class-pattern> reported by reviewdog 🐶
Expected ".hide-stepper" to match pattern "^(spectrum-|is-|u-)[A-Za-z0-9-]+"

--mod-textfield-min-width: var(--mod-numberfield-hidden-stepper-min-inline-size, var(--spectrum-numberfield-hidden-stepper-min-inline-size));


⚠️ [stylelint] <selector-class-pattern> reported by reviewdog 🐶
Expected ".hide-stepper" to match pattern "^(spectrum-|is-|u-)[A-Za-z0-9-]+"

min-inline-size: var(--mod-numberfield-hidden-stepper-min-inline-size, var(--spectrum-numberfield-hidden-stepper-min-inline-size));


⚠️ [stylelint] <selector-class-pattern> reported by reviewdog 🐶
Expected ".hide-stepper" to match pattern "^(spectrum-|is-|u-)[A-Za-z0-9-]+"

&.spectrum-Textfield.is-invalid .spectrum-Textfield-validationIcon {

Copy link
Contributor

github-actions bot commented Apr 21, 2025

🚀 Deployed on https://pr-3681--spectrum-css.netlify.app

@marissahuysentruyt marissahuysentruyt added size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. S2 Spectrum 2 skip_vrt Add to a PR to skip running VRT (but still pass the action) labels Apr 22, 2025
@marissahuysentruyt marissahuysentruyt changed the base branch from spectrum-two to aramos-adobe/css770-infield-button-s2-migration April 22, 2025 12:22
@marissahuysentruyt marissahuysentruyt added the do not merge A flag for a branch indicating it should not be merged. label Apr 22, 2025
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1022-s2-migration-number-field-stepper branch from 55e94b1 to 840fffe Compare April 22, 2025 12:59
isInvalid,
isFocused,
isDisabled,
displayLabel: false,
Copy link
Collaborator Author

@marissahuysentruyt marissahuysentruyt Apr 22, 2025

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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?)

Copy link
Collaborator

@rise-erpelding rise-erpelding Apr 29, 2025

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!

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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) {
Copy link
Collaborator Author

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.

@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1022-s2-migration-number-field-stepper branch from e2354a0 to b1f40f5 Compare April 23, 2025 18:31
@marissahuysentruyt marissahuysentruyt marked this pull request as ready for review April 23, 2025 18:34
@@ -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. */
Copy link
Collaborator Author

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.

Copy link
Collaborator

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.

Copy link
Collaborator Author

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?

Copy link
Collaborator

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?

Comment on lines 43 to 47
--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));
Copy link
Collaborator Author

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,
Copy link
Contributor

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 });
	},

Copy link
Collaborator

@rise-erpelding rise-erpelding Apr 25, 2025

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.

Copy link
Collaborator Author

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. 👍

Copy link
Collaborator Author

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!

df46640

@aramos-adobe
Copy link
Contributor

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.

Base automatically changed from aramos-adobe/css770-infield-button-s2-migration to spectrum-two April 24, 2025 15:58
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1022-s2-migration-number-field-stepper branch from 32555b2 to 785d2f9 Compare April 24, 2025 16:07
@marissahuysentruyt marissahuysentruyt removed the do not merge A flag for a branch indicating it should not be merged. label Apr 24, 2025
Copy link
Collaborator

@rise-erpelding rise-erpelding left a 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,
Copy link
Collaborator

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.

--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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the inline-size on .spectrum-Textfield that includes this --mod-textfield-width is crossed out, maybe we don't need to set this mod? Or maybe we set it to 100% so we don't need to set inline-size on .spectrum-NumberField-textfield?

image

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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);
Copy link
Collaborator

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:
image

Inspecting it seems like that text-to-stepper token is being overridden so it never gets used:
image

Copy link
Collaborator Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

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,
Copy link
Collaborator

@rise-erpelding rise-erpelding Apr 25, 2025

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
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1022-s2-migration-number-field-stepper branch from 785d2f9 to c183290 Compare April 29, 2025 19:28
- 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)
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1022-s2-migration-number-field-stepper branch from c183290 to 107d9a9 Compare April 29, 2025 19:41
- 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
@castastrophe castastrophe force-pushed the spectrum-two branch 3 times, most recently from 7e8269c to 4ce8d19 Compare May 5, 2025 21:18
Copy link
Collaborator

@rise-erpelding rise-erpelding left a 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!

Comment on lines +119 to +121
* 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)
Copy link
Collaborator

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({});
Copy link
Collaborator

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,
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ted-lasso-yes

@@ -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. */
Copy link
Collaborator

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?

Comment on lines +267 to +268
&::before {
content: "";
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Screenshot 2025-05-06 at 4 50 00 PM

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

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);
Copy link
Collaborator

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?

image

image


/* 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);
Copy link
Collaborator

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.

@marissahuysentruyt marissahuysentruyt mentioned this pull request May 6, 2025
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review S2 Spectrum 2 size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants