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(slider): use high contrast properties and fix ramp handle circle #2752

Merged
merged 1 commit into from
May 30, 2024

Conversation

jawinn
Copy link
Collaborator

@jawinn jawinn commented May 13, 2024

Description

Updates the Slider high contrast styles to use all --highcontrast prefixed custom properties. It was noticed that some were being set with --spectrum prefixed custom properties. Some were duplicative, as they already had --highcontrast prefixed custom properties being set (which take precendence in the var() fallback chains). A few needed a rename.

In the process, it was also found that the ramp variant was missing the handle outer circle (gap between the ramp background and handle), that created contrast between the handle in the background. This was particularly noticeable on hover/active, as the handle was blending into the same ramp highlight color. The fix was already present but the nested .spectrum-Slider--ramp selector was missing an & to make it the same element as the .spectrum-Slider

CSS-760

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

  • Take a look at the Ramp story in forced-colors mode. The gap around the handle should be visible.
  • There are no other changes in forced-colors mode. Make sure to check filled, filled offset, hover, active, and keyboard focus.

Review note: The only changes made here are within the @media (forced-colors: active). The rest of the files modified are automatic prettier fixes, including changes to whitespace.

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

"Ramp" variant high contrast bug before and after fix:

before after

To-do list

  • I have read the contribution guidelines.
  • 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 May 13, 2024

🦋 Changeset detected

Latest commit: c233023

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/slider 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 May 13, 2024

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

Copy link
Contributor

github-actions bot commented May 13, 2024

File metrics

Summary

Total size: 4.64 MB*
Total change (Δ): ⬇ 1.94 KB (-0.04%)

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

Package Size Δ
slider 32.34 KB ⬇ 0.66 KB

Details

slider

File Head Base Δ
index-base.css 30.00 KB 30.64 KB ⬇ 0.66 KB (-2.11%)
index-theme.css 2.92 KB 2.92 KB -
index-vars.css 32.34 KB 32.98 KB ⬇ 0.66 KB (-1.96%)
index.css 32.34 KB 32.98 KB ⬇ 0.66 KB (-1.96%)
metadata.json 14.17 KB 14.17 KB ⬇ < 0.01 KB (-0.01%)
themes/express.css 1.76 KB 1.76 KB -
themes/spectrum.css 1.73 KB 1.73 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.

@jawinn jawinn force-pushed the jawinn/css-760-slider-hcm-properties branch from d44be12 to 540d7e8 Compare May 13, 2024 16:09
@jawinn jawinn requested review from mdt2 and rise-erpelding May 13, 2024 16:16
Copy link
Collaborator

@mdt2 mdt2 left a comment

Choose a reason for hiding this comment

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

Looks great!

@jawinn jawinn force-pushed the jawinn/css-760-slider-hcm-properties branch from 540d7e8 to e622311 Compare May 22, 2024 21:35
Copy link
Collaborator

@mdt2 mdt2 left a comment

Choose a reason for hiding this comment

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

Took another peek after the rebase conflicts and this is still looking good! Also checked in RTL. Nice work!

@jawinn jawinn force-pushed the jawinn/css-760-slider-hcm-properties branch from e622311 to e29a340 Compare May 30, 2024 19:31
Updates the Slider high contrast styles to use the --highcontrast
prefixed custom properties. Some were being set with --spectrum prefixed
custom properties.

In the process, it was also found that the ramp variant was missing the
handle outer circle (gap between the ramp background and handle), that
created contrast between the handle in the background. This was
particularly noticeable on hover/active, as the handle was blending into
the same ramp highlight color. The fix was already present but the
nested .spectrum-Slider--ramp selector was missing an '&' to make it the
same element as the .spectrum-Slider
@pfulton pfulton force-pushed the jawinn/css-760-slider-hcm-properties branch from e29a340 to c233023 Compare May 30, 2024 19:55
@pfulton pfulton added the run_vrt For use on PRs looking to kick off VRT label May 30, 2024
@pfulton pfulton merged commit 4465b9a into main May 30, 2024
16 of 22 checks passed
@pfulton pfulton deleted the jawinn/css-760-slider-hcm-properties branch May 30, 2024 20:03
@github-actions github-actions bot mentioned this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review ready-to-merge run_vrt For use on PRs looking to kick off VRT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants