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

fix(stepper): corrects invalid button color border to match field in windows high contrast mode #3103

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

cdransf
Copy link
Member

@cdransf cdransf commented Sep 11, 2024

Description

  • This corrects the border color for the stepper component infield buttons by setting the border color property to Highlight when the parent stepper component is invalid.

How and where has this been tested?

  • Verified in assistiv labs Windows High Contrast Mode

Validation steps

  1. Run Storybook locally (or reference the link for this PR).
  2. Navigate to Assistiv Labs and start a Windows High Contrast Mode VM (if running the branch locally, you'll need to enable the Assistiv Labs tunnel to access Storybook).
  3. Enable Windows High Contrast Mode.
  4. Navigate to the Stepper component in Storybook.
  5. Verify that the stepper button border color matches the parent stepper field when the component is set to invalid.

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

Screenshot 2024-09-12 at 3 53 11 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. ✨

@cdransf cdransf added run_vrt For use on PRs looking to kick off VRT ready-for-review labels Sep 11, 2024
Copy link

changeset-bot bot commented Sep 11, 2024

🦋 Changeset detected

Latest commit: 0aeeedd

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

Copy link
Contributor

github-actions bot commented Sep 11, 2024

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

Copy link
Contributor

github-actions bot commented Sep 11, 2024

File metrics

Summary

Total size: 4.10 MB*
Total change (Δ): ⬆ 0.34 KB (0.01%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
stepper 20.65 KB ⬆ 0.11 KB

Details

stepper

File Head Base Δ
index-base.css 17.25 KB 17.14 KB ⬆ 0.11 KB (0.64%)
index-theme.css 4.01 KB 4.01 KB -
index-vars.css 20.65 KB 20.54 KB ⬆ 0.11 KB (0.54%)
index.css 20.65 KB 20.54 KB ⬆ 0.11 KB (0.54%)
themes/express.css 2.33 KB 2.33 KB -
themes/spectrum.css 2.29 KB 2.29 KB -
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@cdransf cdransf closed this Sep 11, 2024
@cdransf cdransf reopened this Sep 11, 2024
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.

I flicked the Disabled switch practically as an afterthought while Invalid was on and discovered this color mismatch 🙀 :
image

In this situation, disabled styling should win and that's what should be applied here. I'm wondering if we add --highcontrast-infield-button-border-color: GrayText; to .is-disabled and switch &.is-disabled and &.is-invalid, would that work? (Something like this:)

		&.is-invalid {
			--highcontrast-stepper-border-color: Highlight;
			--highcontrast-stepper-border-color-hover: Highlight;
			--highcontrast-stepper-border-color-focus: Highlight;
			--highcontrast-stepper-border-color-focus-hover: Highlight;
			--highcontrast-stepper-border-color-keyboard-focus: Highlight;
			--highcontrast-infield-button-border-color: Highlight;
		}

		&.is-disabled {
			--highcontrast-stepper-border-color: GrayText;
			--highcontrast-infield-button-border-color: GrayText;
			--highcontrast-stepper-buttons-border-width: var(--mod-stepper-border-width, var(--spectrum-stepper-border-width));
		}

It changes the colors for me but I'm hoping that there aren't any other implications I'm missing of switching those selectors around?

@cdransf
Copy link
Member Author

cdransf commented Sep 12, 2024

I flicked the Disabled switch practically as an afterthought while Invalid was on and discovered this color mismatch 🙀 : image

In this situation, disabled styling should win and that's what should be applied here. I'm wondering if we add --highcontrast-infield-button-border-color: GrayText; to .is-disabled and switch &.is-disabled and &.is-invalid, would that work? (Something like this:)

		&.is-invalid {
			--highcontrast-stepper-border-color: Highlight;
			--highcontrast-stepper-border-color-hover: Highlight;
			--highcontrast-stepper-border-color-focus: Highlight;
			--highcontrast-stepper-border-color-focus-hover: Highlight;
			--highcontrast-stepper-border-color-keyboard-focus: Highlight;
			--highcontrast-infield-button-border-color: Highlight;
		}

		&.is-disabled {
			--highcontrast-stepper-border-color: GrayText;
			--highcontrast-infield-button-border-color: GrayText;
			--highcontrast-stepper-buttons-border-width: var(--mod-stepper-border-width, var(--spectrum-stepper-border-width));
		}

It changes the colors for me but I'm hoping that there aren't any other implications I'm missing of switching those selectors around?

Oohh nice catch! ✨

I dropped in the changes you suggested and they look better to me:

Screenshot 2024-09-12 at 3 53 11 PM

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.

These changes look good! I'm glad all these components are getting updates in WHCM!

One more thing I noticed about the stepper that may be better off as a separate card:
image

Any of the disabled states here have the funny mismatching background color (the one that's only visible when you use Assistiv Labs) where the number is, similar to what you were seeing on the negative button before in #3078. Looks like it's visible in textfield too, which makes sense:

image

@cdransf cdransf force-pushed the cdransf/stepper-border-color-fix branch from 747220e to 0aeeedd Compare September 13, 2024 14:39
@cdransf
Copy link
Member Author

cdransf commented Sep 13, 2024

These changes look good! I'm glad all these components are getting updates in WHCM!

One more thing I noticed about the stepper that may be better off as a separate card: image

Any of the disabled states here have the funny mismatching background color (the one that's only visible when you use Assistiv Labs) where the number is, similar to what you were seeing on the negative button before in #3078. Looks like it's visible in textfield too, which makes sense:

image

Thank you! Nice catch — I'll open up a ticket to look at that as well. ✨

@cdransf cdransf merged commit cf61a13 into main Sep 13, 2024
14 checks passed
@cdransf cdransf deleted the cdransf/stepper-border-color-fix branch September 13, 2024 15:18
@github-actions github-actions bot mentioned this pull request Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review run_vrt For use on PRs looking to kick off VRT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants