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): high contrast improvements for filled track #3952

Merged
merged 3 commits into from Feb 12, 2024

Conversation

mdt2
Copy link
Collaborator

@mdt2 mdt2 commented Jan 18, 2024

Description

  • Uses Spectrum CSS Slider version 4.2.1

Related issue(s)

Motivation and context

Add Windows High Contrast Mode improvements for sliders with a filled track.

How has this been tested?

  • Test that Sliders are rendering correctly on the docs page
  • Test that Sliders are rendering correctly in Storybook
  • VRTs are passing

Screenshots

Before:
Screen Shot 2024-01-18 at 4 35 34 PM

After:
Screen Shot 2024-01-18 at 4 33 30 PM

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

Copy link

github-actions bot commented Jan 18, 2024

Tachometer results

Chrome

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 480 kB 109.12ms - 111.16ms - unsure 🔍
-1% - +2%
-0.87ms - +1.86ms
branch 472 kB 108.73ms - 110.56ms unsure 🔍
-2% - +1%
-1.86ms - +0.87ms
-
Firefox

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 480 kB 219.68ms - 228.92ms - unsure 🔍
-2% - +3%
-5.30ms - +7.34ms
branch 472 kB 218.97ms - 227.59ms unsure 🔍
-3% - +2%
-7.34ms - +5.30ms
-

@mdt2
Copy link
Collaborator Author

mdt2 commented Jan 19, 2024

The failing VRTs in the Slider component are expected as part of the change.

@Westbrook
Copy link
Collaborator

Yep, the changes look like what I'd expect from you change. Can you take a look at this part of the docs to get the cache updated?

@mdt2
Copy link
Collaborator Author

mdt2 commented Jan 19, 2024

@Westbrook thanks for that link! I updated the cache key, and after pushing realized I'm out of sync with main so I'll need to rebase, but before I do I'm now seeing that some Toast stories are removed in the VRTs which is definitely unexpected for this one. Any idea what could be causing that?

@Westbrook
Copy link
Collaborator

Any idea what could be causing that?

Sometimes failures cause failures in the VRTs and you need to update the cache value twice for it to go back to clean. Sadly.

@mdt2
Copy link
Collaborator Author

mdt2 commented Jan 19, 2024

Sometimes failures cause failures in the VRTs and you need to update the cache value twice for it to go back to clean. Sadly.

Ok thanks. I'll move forward with updating the cache value to the new one

Copy link
Collaborator

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Sorry, I rebased this thinking it was ready to go, but we need to update to a non-beta release before we can merge. Keep an eye out for that when updating the version. After that, we should be good to go here. Thanks for taking care of this!

@@ -83,7 +83,7 @@
"@spectrum-web-components/theme": "^0.40.3"
},
"devDependencies": {
"@spectrum-css/slider": "^4.1.19"
"@spectrum-css/slider": "4.2.1-beta.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have this gotten a full release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not yet, on our side this work is pending re-review now that we've validated the changes here. I'll reach out to the team again to try to get some eyes on it.

@Westbrook
Copy link
Collaborator

Would love to get a stable release on this if we're ready.

@mdt2
Copy link
Collaborator Author

mdt2 commented Feb 12, 2024

Would love to get a stable release on this if we're ready.

@Westbrook we were able to get this merged and released today and I've updated the PR with the new version 👍

Copy link
Collaborator

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Thanks for driving this home. Looking forward to including it in this week's release.

@Westbrook Westbrook merged commit 782560d into main Feb 12, 2024
47 of 48 checks passed
@Westbrook Westbrook deleted the mdt2/css-638-slider-whcm branch February 12, 2024 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants