Skip to content

Conversation

@pfulton
Copy link
Collaborator

@pfulton pfulton commented Feb 6, 2023

Description

Migrates Slider to the new tokens system. Using beta release. Hold for full release.

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.

@pfulton
Copy link
Collaborator Author

pfulton commented Feb 6, 2023

Spectrum CSS PR: adobe/spectrum-css#1547

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Tachometer results

Chrome

action-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 345 kB 44.27ms - 46.26ms - unsure 🔍
-3% - +3%
-1.28ms - +1.38ms
branch 344 kB 44.33ms - 46.10ms unsure 🔍
-3% - +3%
-1.38ms - +1.28ms
-

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 708 kB 250.70ms - 257.12ms - unsure 🔍
-1% - +2%
-3.31ms - +5.00ms
branch 709 kB 250.42ms - 255.70ms unsure 🔍
-2% - +1%
-5.00ms - +3.31ms
-

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 484 kB 133.28ms - 137.55ms - unsure 🔍
-3% - +1%
-4.66ms - +2.05ms
branch 484 kB 134.14ms - 139.31ms unsure 🔍
-2% - +3%
-2.05ms - +4.66ms
-

field-label permalink

Version Bytes Avg Time vs remote vs branch
npm latest 368 kB 60.88ms - 63.59ms - unsure 🔍
-3% - +2%
-1.90ms - +1.43ms
branch 367 kB 61.50ms - 63.45ms unsure 🔍
-2% - +3%
-1.43ms - +1.90ms
-

illustrated-message permalink

Version Bytes Avg Time vs remote vs branch
npm latest 383 kB 63.09ms - 65.08ms - unsure 🔍
-1% - +4%
-0.40ms - +2.45ms
branch 382 kB 62.04ms - 64.08ms unsure 🔍
-4% - +1%
-2.45ms - +0.40ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 414 kB 316.40ms - 324.74ms - unsure 🔍
-3% - +1%
-8.33ms - +4.68ms
branch 413 kB 317.40ms - 327.39ms unsure 🔍
-1% - +3%
-4.68ms - +8.33ms
-

meter permalink

Version Bytes Avg Time vs remote vs branch
npm latest 382 kB 108.06ms - 111.82ms - unsure 🔍
-3% - +2%
-3.16ms - +2.05ms
branch 381 kB 108.69ms - 112.30ms unsure 🔍
-2% - +3%
-2.05ms - +3.16ms
-

overlay permalink

Version Bytes Avg Time vs remote vs branch
npm latest 433 kB 94.70ms - 98.46ms - unsure 🔍
-5% - +3%
-4.43ms - +2.82ms
branch 432 kB 94.29ms - 100.49ms unsure 🔍
-3% - +5%
-2.82ms - +4.43ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 564 kB 756.78ms - 785.25ms - unsure 🔍
-4% - +2%
-27.72ms - +15.23ms
branch 565 kB 761.18ms - 793.34ms unsure 🔍
-2% - +4%
-15.23ms - +27.72ms
-

popover permalink

Version Bytes Avg Time vs remote vs branch
npm latest 342 kB 43.43ms - 46.48ms - unsure 🔍
-2% - +6%
-0.97ms - +2.48ms
branch 341 kB 43.39ms - 45.00ms unsure 🔍
-5% - +2%
-2.48ms - +0.97ms
-

progress-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 380 kB 77.02ms - 79.89ms - unsure 🔍
-4% - +1%
-3.03ms - +0.60ms
branch 379 kB 78.57ms - 80.77ms unsure 🔍
-1% - +4%
-0.60ms - +3.03ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 447 kB 198.99ms - 204.10ms - unsure 🔍
-1% - +2%
-2.83ms - +4.50ms
branch 446 kB 198.09ms - 203.34ms unsure 🔍
-2% - +1%
-4.50ms - +2.83ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 642 kB 2088.50ms - 2221.88ms - unsure 🔍
-5% - +4%
-98.91ms - +90.14ms
branch 643 kB 2092.59ms - 2226.56ms unsure 🔍
-4% - +5%
-90.14ms - +98.91ms
-

tooltip permalink

Version Bytes Avg Time vs remote vs branch
npm latest 355 kB 47.59ms - 50.01ms - unsure 🔍
-3% - +4%
-1.28ms - +1.90ms
branch 354 kB 47.46ms - 49.52ms unsure 🔍
-4% - +3%
-1.90ms - +1.28ms
-
Firefox

action-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 345 kB 127.96ms - 142.28ms - unsure 🔍
-5% - +11%
-5.85ms - +14.53ms
branch 344 kB 123.52ms - 138.04ms unsure 🔍
-11% - +4%
-14.53ms - +5.85ms
-

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 708 kB 414.55ms - 446.29ms - unsure 🔍
-3% - +7%
-12.27ms - +28.67ms
branch 709 kB 409.29ms - 435.15ms unsure 🔍
-7% - +3%
-28.67ms - +12.27ms
-

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 484 kB 230.90ms - 249.98ms - unsure 🔍
-7% - +4%
-16.97ms - +10.61ms
branch 484 kB 233.66ms - 253.58ms unsure 🔍
-4% - +7%
-10.61ms - +16.97ms
-

field-label permalink

Version Bytes Avg Time vs remote vs branch
npm latest 368 kB 140.40ms - 161.04ms - unsure 🔍
-4% - +16%
-4.97ms - +21.93ms
branch 367 kB 133.62ms - 150.86ms unsure 🔍
-14% - +3%
-21.93ms - +4.97ms
-

illustrated-message permalink

Version Bytes Avg Time vs remote vs branch
npm latest 383 kB 143.00ms - 154.48ms - unsure 🔍
-8% - +2%
-12.82ms - +3.26ms
branch 382 kB 147.89ms - 159.15ms unsure 🔍
-2% - +9%
-3.26ms - +12.82ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 414 kB 521.75ms - 543.53ms - faster ✔
3% - 9%
18.33ms - 53.51ms
branch 413 kB 554.75ms - 582.37ms slower ❌
3% - 10%
18.33ms - 53.51ms
-

meter permalink

Version Bytes Avg Time vs remote vs branch
npm latest 382 kB 221.61ms - 242.95ms - unsure 🔍
-8% - +6%
-19.67ms - +13.79ms
branch 381 kB 222.33ms - 248.11ms unsure 🔍
-6% - +9%
-13.79ms - +19.67ms
-

overlay permalink

Version Bytes Avg Time vs remote vs branch
npm latest 433 kB 171.24ms - 196.04ms - unsure 🔍
-12% - +8%
-23.42ms - +15.02ms
branch 432 kB 173.15ms - 202.53ms unsure 🔍
-8% - +13%
-15.02ms - +23.42ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 564 kB 1063.25ms - 1097.67ms - unsure 🔍
-2% - +3%
-24.30ms - +31.22ms
branch 565 kB 1055.22ms - 1098.78ms unsure 🔍
-3% - +2%
-31.22ms - +24.30ms
-

popover permalink

Version Bytes Avg Time vs remote vs branch
npm latest 342 kB 95.89ms - 111.11ms - unsure 🔍
-14% - +7%
-15.12ms - +7.84ms
branch 341 kB 98.54ms - 115.74ms unsure 🔍
-8% - +15%
-7.84ms - +15.12ms
-

progress-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 380 kB 150.32ms - 169.20ms - unsure 🔍
-12% - +6%
-20.52ms - +9.40ms
branch 379 kB 153.72ms - 176.92ms unsure 🔍
-6% - +13%
-9.40ms - +20.52ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 447 kB 364.71ms - 387.65ms - unsure 🔍
-6% - +2%
-24.17ms - +8.49ms
branch 446 kB 372.39ms - 395.65ms unsure 🔍
-2% - +6%
-8.49ms - +24.17ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 642 kB 2159.68ms - 2174.24ms - unsure 🔍
-1% - +0%
-11.16ms - +7.88ms
branch 643 kB 2162.48ms - 2174.72ms unsure 🔍
-0% - +1%
-7.88ms - +11.16ms
-

tooltip permalink

Version Bytes Avg Time vs remote vs branch
npm latest 355 kB 92.67ms - 105.45ms - unsure 🔍
-13% - +7%
-13.26ms - +7.10ms
branch 354 kB 94.22ms - 110.06ms unsure 🔍
-7% - +14%
-7.10ms - +13.26ms
-

@mlogsdon18 mlogsdon18 force-pushed the slider-core-tokens branch 3 times, most recently from 96efa59 to 77f48bc Compare March 24, 2023 13:38
Westbrook added a commit that referenced this pull request Apr 20, 2023
Westbrook added a commit that referenced this pull request Apr 20, 2023
Westbrook added a commit that referenced this pull request Apr 20, 2023
Westbrook added a commit that referenced this pull request Apr 20, 2023
@mlogsdon18 mlogsdon18 force-pushed the slider-core-tokens branch 4 times, most recently from 2720aa4 to f30913d Compare May 10, 2023 19:06
@mlogsdon18 mlogsdon18 marked this pull request as ready for review May 11, 2023 19:28
@mlogsdon18 mlogsdon18 force-pushed the slider-core-tokens branch from f30913d to e2487a0 Compare May 15, 2023 15:26
@mlogsdon18
Copy link
Contributor

@Westbrook ok this one is ready for your review! I believe all of the VRT failures are due to some changes in spacing that are expected

@Westbrook
Copy link
Contributor

Looks like that "label" and the "value" receive different styles which cause them not to share the same baseline. This is particularly evident in the "large" and "extra large" versions as can be exhibited in the CSS preview as well: https://pr-1696--spectrum-css.netlify.app/slider.html

image

@Westbrook
Copy link
Contributor

Westbrook commented May 15, 2023

This VRT points out a bug in Field Label where we are not applying a --highcontrast- custom property to https://github.com/adobe/spectrum-css/blob/main/components/fieldlabel/index.css#L160-L169

@Westbrook
Copy link
Contributor

Westbrook commented May 15, 2023

@mlogsdon18
Copy link
Contributor

mlogsdon18 commented May 16, 2023

Looks like that "label" and the "value" receive different styles which cause them not to share the same baseline. This is particularly evident in the "large" and "extra large" versions as can be exhibited in the CSS preview as well: https://pr-1696--spectrum-css.netlify.app/slider.html

image

@Westbrook This was checked and approved by PJ who recognized that this pixel or two difference was due to the line height of the label that's being applied. We have a token in place that moves the slider up some negative margin to account for that and design deemed that as acceptable. Are you thinking it shouldn't be?
Screenshot 2023-05-16 at 10 18 22 AM

@mlogsdon18 mlogsdon18 force-pushed the slider-core-tokens branch from a96b007 to 1d46dbf Compare May 17, 2023 12:37
@mlogsdon18
Copy link
Contributor

@Westbrook I went through and checked the custom CSS you all are implementing. Most of it is specific to SWC and wasn't in Spectrum CSS in the first place so we should be good there (your first two links). I did update the third link to reference the new variable coming in from Spectrum CSS just to eliminate an unneeded middle variable.

I also went through the custom CSS file and removed some CSS that is no longer needed due to it being implemented in Specturm CSS. Specifically the cursor: pointer on the track and the border transition for the handle.

I will put up a PR for the bug fix for Field Label in Spectrum CSS and hopefully we can get that in quickly!

@mlogsdon18 mlogsdon18 marked this pull request as draft May 17, 2023 12:45
@Westbrook
Copy link
Contributor

@mlogsdon18 this is looking good! If we're able to get a stable release for this (and, hopefully, the Field Label) CSS then we'll be ready to clean up the VRTs, etc and merge this. Thanks for following up on everything and getting those ready to go. 🙇🏼‍♂️

@mlogsdon18 mlogsdon18 force-pushed the slider-core-tokens branch from 86dc6b6 to 55db04d Compare May 18, 2023 20:33
@mlogsdon18 mlogsdon18 marked this pull request as ready for review May 18, 2023 20:33
@mlogsdon18
Copy link
Contributor

@Westbrook WOO! I got some full releases and they have been updated in this PR

  • Slider is now at 4.0.0
  • Field Label is now at 7.0.0

@Westbrook Westbrook force-pushed the slider-core-tokens branch from e7bb606 to 5150c9e Compare May 19, 2023 19:50
@Westbrook Westbrook force-pushed the slider-core-tokens branch from f01d40f to 1dbc4bb Compare May 19, 2023 20:53
Copy link
Contributor

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

@mlogsdon18 thanks for pushing this one through. Lots of great updates coming to our consumers here. Happy Friday!!

@Westbrook Westbrook merged commit f52efc0 into main May 19, 2023
@Westbrook Westbrook deleted the slider-core-tokens branch May 19, 2023 21:01
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.

4 participants