Skip to content
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

refactor(stepper): standardize selectors and passthroughs usage #3558

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

marissahuysentruyt
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt commented Feb 14, 2025

Description

During #3536, we found that the stepper had more bugs particularly in dark mode, and particularly when you hovered over other states, like focus or keyboard-focus. This PR should revamp the CSS so that the stepper states are triggered in the expected way, where hovering over any of the nested subcomponents triggers the correct border color changes to all subcomponents. As an example, hovering over the stepper buttons should trigger a border color change in the stepper buttons, AS WELL AS trigger the border color on the stepper textfield.

The PR also adds focus + hover & keyboardFocus + hover Chromatic test cases to ensure we don't miss that state in the future.

Jira/Specs

CSS-1127
Also related to CSS-1112

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 or visit the deploy preview.
  • Verify nothing on the stepper docs page looks broken. No visual changes were made that should have effected any of the these components.
  • Visit the chromatic testing preview
  • For each state in the default test case, verify that hovering over the stepper's textfield only changes the border colors of the stepper textfield AND the stepper buttons. The borders should change to the same color.
  • Similarly, for each state in the default test case, verify that hovering over the stepper's buttons only changes the border colors of the stepper textfield AND the stepper buttons. The borders should change to the same color.
  • Repeat this process for the quiet, invalid, and quiet+invalid variants. Border colors should change together regardless of where a user hovers their cursor.
  • Repeat this process for dark mode as well.
  • Verify all behaviors are the same in the Spectrum 1 context.
  • Verify all behaviors are the same in the Express context.

A couple of notes while reviewing:

  • Some of the more subtle border color changes (like focus to focus+hover) are more easily seen in dark mode.
  • The disabled styles should win out every time. Hover, focus and keyboardFocus styles should not be present if a stepper is disabled.

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

Before 🚫
Screenshot 2025-02-14 at 4 40 21 PM
Screenshot 2025-02-14 at 4 40 06 PM
Screenshot 2025-02-14 at 4 38 12 PM
Screenshot 2025-03-04 at 12 35 02 PM

After ✅
Screenshot 2025-03-04 at 12 33 46 PM
Screenshot 2025-03-04 at 12 34 31 PM
Screenshot 2025-03-04 at 12 34 44 PM
Screenshot 2025-03-04 at 12 35 23 PM

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

Copy link

changeset-bot bot commented Feb 14, 2025

🦋 Changeset detected

Latest commit: 876ee12

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

This PR includes changesets to release 1 package
Name Type
@spectrum-css/stepper Minor

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

@marissahuysentruyt marissahuysentruyt self-assigned this Feb 14, 2025
@marissahuysentruyt marissahuysentruyt added the wip This is a work in progress, don't judge. label Feb 14, 2025
@marissahuysentruyt marissahuysentruyt changed the base branch from main to marissahuysentruyt/css-1112-background-color-form-elements-fix February 14, 2025 21:22
Copy link
Contributor

github-actions bot commented Feb 14, 2025

File metrics

Summary

Total size: 2.25 MB*
No change in file sizes

Package Size Minified Gzipped
button 38.55 KB 36.64 KB 4.20 KB
stepper 18.71 KB 17.82 KB 2.35 KB

File change details

button

Filename Head Minified Gzipped Compared to base
index-base.css 31.05 KB 29.48 KB 3.80 KB -
index-theme.css 12.40 KB 12.01 KB 1.14 KB -
index.css 38.55 KB 36.64 KB 4.20 KB -
metadata.json 19.88 KB - - 🟢 ⬇ 0.08 KB
themes/express.css 9.68 KB 9.38 KB 1.06 KB -
themes/spectrum-two.css 9.44 KB 9.15 KB 1.07 KB -
themes/spectrum.css 9.76 KB 9.46 KB 1.08 KB -

stepper

Filename Head Minified Gzipped Compared to base
index-base.css 17.22 KB 16.41 KB 2.21 KB 🔴 ⬆ 0.56 KB
index-theme.css 3.03 KB 2.95 KB 0.70 KB 🔴 ⬆ 0.27 KB
index.css 18.71 KB 17.82 KB 2.35 KB 🔴 ⬆ 0.36 KB
metadata.json 9.35 KB - - 🔴 ⬆ 0.40 KB
themes/express.css 2.36 KB 2.28 KB 0.68 KB 🔴 ⬆ 0.30 KB
themes/spectrum-two.css 2.03 KB 1.97 KB 0.64 KB 🟢 ⬇ 0.20 KB
themes/spectrum.css 2.12 KB 2.05 KB 0.66 KB 🟢 ⬇ 0.11 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.

@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from 89ab810 to 408ed9a Compare February 18, 2025 20:19
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-stepper-border-focus-hover-color branch from 6ac2359 to 9632409 Compare February 18, 2025 20:23
@marissahuysentruyt marissahuysentruyt changed the title Marissahuysentruyt/fix stepper border focus hover color fix(stepper): border focus hover color Feb 18, 2025
@marissahuysentruyt marissahuysentruyt marked this pull request as ready for review February 18, 2025 20:25
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from 408ed9a to a09845c Compare February 18, 2025 21:02
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-stepper-border-focus-hover-color branch from 9632409 to f09bfc4 Compare February 18, 2025 21:14
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch from a09845c to 220b448 Compare February 26, 2025 15:27
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-stepper-border-focus-hover-color branch from f09bfc4 to 6f014e1 Compare February 26, 2025 15:28
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1112-background-color-form-elements-fix branch 2 times, most recently from 4482b05 to a73d206 Compare February 27, 2025 14:57
Base automatically changed from marissahuysentruyt/css-1112-background-color-form-elements-fix to main February 27, 2025 15:06
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-stepper-border-focus-hover-color branch from 6f014e1 to ce36838 Compare February 27, 2025 15:08
Copy link
Contributor

github-actions bot commented Feb 27, 2025

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

@marissahuysentruyt marissahuysentruyt marked this pull request as draft February 28, 2025 16:57
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-stepper-border-focus-hover-color branch 3 times, most recently from 5ff92bd to 4243813 Compare March 3, 2025 19:35
@@ -17,21 +17,17 @@
--spectrum-stepper-border-color-default: var(--spectrum-gray-500);
--spectrum-stepper-border-color-hover: var(--spectrum-gray-600);
--spectrum-stepper-border-color-focus: var(--spectrum-gray-800);
--spectrum-stepper-border-color-focus-hover: var(--spectrum-gray-800);
--spectrum-stepper-border-color-focus-hover: var(--spectrum-gray-900);
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 s2 token specs say gray-900 for focus+hover

Comment on lines 25 to 29
--spectrum-stepper-buttons-border-color: var(--spectrum-gray-500);
--spectrum-stepper-buttons-background-color: var(--spectrum-gray-100);
--spectrum-stepper-buttons-border-color-hover: var(--spectrum-gray-600);
--spectrum-stepper-buttons-border-color-focus: var(--spectrum-gray-800);
--spectrum-stepper-buttons-border-color-keyboard-focus: var(--spectrum-gray-900);
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 found it easier to use the --mod-stepper-border-color all over, in tandem with the --mod-infield-button-border-color to target the stepper buttons. However, if we remove these, would this be a breaking change?


--spectrum-stepper-button-border-width: var(--spectrum-border-width-100);

/** Invalid **/
--spectrum-stepper-border-color-invalid: var(--spectrum-negative-border-color-default);
--spectrum-stepper-border-color-hover-invalid: var(--spectrum-negative-border-color-hover);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looked like we were missing the invalid hover color (token specs)

Comment on lines +124 to +125
&.is-focused,
&:focus {
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 additional :not(.is-disabled) was making it really, really difficult to target these mods and nested styles. Removing them helped, and I don't think I saw any regressions with the disabled states. Please double check that all of the disabled styles are winning out and rendering, even though I removed the :not().

@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-stepper-border-focus-hover-color branch from 4243813 to 570459d Compare March 4, 2025 15:44
Comment on lines +24 to +26
&.is-disabled:not(.spectrum-Stepper--quiet) {
--spectrum-stepper-border-color-disabled: transparent;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [stylelint] <spectrum-tools/theme-alignment> reported by reviewdog 🐶
Selector "&.is-disabled:not(.spectrum-Stepper--quiet)" is not used or defined in the base file (themes/spectrum-two.css).

@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-stepper-border-focus-hover-color branch from 570459d to 6bf8376 Compare March 4, 2025 17:07
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-stepper-border-focus-hover-color branch from b9ad2b2 to de13e95 Compare March 4, 2025 17:40
@marissahuysentruyt marissahuysentruyt added bug Results from a bug in the CSS implementation size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. run_vrt For use on PRs looking to kick off VRT and removed wip This is a work in progress, don't judge. labels Mar 4, 2025
@marissahuysentruyt marissahuysentruyt marked this pull request as ready for review March 4, 2025 17:42
- fixes the focus & focus hover states, so that hovering over either
element (the textfield OR the stepper buttons) will trigger the SAME
border color for both elements.
- removed some of the :not(disabled) to simplify selectors.
- add keyboardfocused+hovered test
- keyboardfocused+hovered states showcase hovering over any part of the
element triggers the SAME border color for the textfield and the infield
buttons
- corrects the s1 borders for disabled steppers in quiet & default
variants
- corrects the focus+hover state in express
- metadata updated and removal of code/TODO comments
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/fix-stepper-border-focus-hover-color branch from de13e95 to 3206b0b Compare March 4, 2025 19:30
@marissahuysentruyt marissahuysentruyt added the low priority Not a critical update or fix; can be deprioritized if necessary label Mar 6, 2025
--mod-textfield-border-color: var(--highcontrast-stepper-border-color-focus-hover, var(--mod-stepper-border-color-focus-hover, var(--spectrum-stepper-border-color-focus-hover)));
}
}
&:not(.is-disabled).is-keyboardFocused,
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 keyboard-focus still has differing border colors in the textfield vs. stepper buttons.

@marissahuysentruyt marissahuysentruyt removed run_vrt For use on PRs looking to kick off VRT ready-for-review labels Mar 7, 2025
@marissahuysentruyt marissahuysentruyt marked this pull request as draft March 7, 2025 19:09
@marissahuysentruyt marissahuysentruyt added the do not merge A flag for a branch indicating it should not be merged. label Mar 11, 2025
@castastrophe castastrophe force-pushed the main branch 11 times, most recently from c68f4e3 to d2272ea Compare March 12, 2025 21:56
@marissahuysentruyt marissahuysentruyt changed the title fix(stepper): border focus hover color refactor(stepper): standardize selectors and passthroughs usage Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Results from a bug in the CSS implementation do not merge A flag for a branch indicating it should not be merged. low priority Not a critical update or fix; can be deprioritized if necessary size-3 M ~18-30hrs; moderate effort or complexity, several work days needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant