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

feat(menu): convert to core tokens #3254

Merged
merged 26 commits into from Aug 14, 2023
Merged

feat(menu): convert to core tokens #3254

merged 26 commits into from Aug 14, 2023

Conversation

najikahalsema
Copy link
Collaborator

@najikahalsema najikahalsema commented May 30, 2023

Description

Convert to core tokens. Things that still need to be done/looked at:

  • checkmark margin via -spectrum-text-to-visual-100 is 8 when it should be 10 to keep the selected item from narrowing
  • Menu divider- should it touch the sides in this new version?
  • Menu margins for popover is pretty narrow— but putting a popover on the styling for the css makes it look nicer. can we get clarification on whether or not the sp-menu element should have margins?
  • Focus/hover state isn’t getting applied to the menu item background. I think this is due to the “custom vars”
  • add "Sizes" stories to the Menu package
  • pass this.size down from Picker.ts into the <sp-menu> child in its shadow DOM
  • add a set of "Sizes Open" stories to Picker so that the above change is included in VRTs
  • add a set of "Sizes Open" stories to Action Menu and Split Button
  • update the VRT cache with the updated and new stories
  • get a stable release from the CSS team

Related issue(s)

Motivation and context

upgrading to core tkoens :)

How has this been tested?

  • Test case 1

    1. Go here
    2. Compare the changes between the old and new versions. Note the checkmark is now on the left, and that the focus-visible is... well, visible.
  • Test case 2

    1. Go here and see that the WHCM doesn't have that strange black line when a menu item is focused
    2. Compare the changes between the old and new versions
  • Test case 3

    1. Go here
    2. See that the menu divider is visible
  • Test Case 4

    1. Go here
    2. Confirm that the space after the chevron is equal to the previous version

Screenshots (if appropriate)

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.

@github-actions
Copy link

github-actions bot commented May 30, 2023

Tachometer results

Chrome

action-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 476 kB 159.93ms - 164.13ms - unsure 🔍
-2% - +1%
-3.44ms - +2.34ms
branch 472 kB 160.60ms - 164.56ms unsure 🔍
-1% - +2%
-2.34ms - +3.44ms
-

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 748 kB 252.76ms - 260.76ms - faster ✔
7% - 10%
18.24ms - 28.90ms
branch 758 kB 276.80ms - 283.85ms slower ❌
7% - 11%
18.24ms - 28.90ms
-

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 499 kB 154.35ms - 160.07ms - unsure 🔍
-3% - +2%
-4.25ms - +2.92ms
branch 495 kB 155.71ms - 160.03ms unsure 🔍
-2% - +3%
-2.92ms - +4.25ms
-

illustrated-message permalink

Version Bytes Avg Time vs remote vs branch
npm latest 395 kB 63.33ms - 65.98ms - unsure 🔍
-0% - +5%
-0.02ms - +3.09ms
branch 391 kB 62.31ms - 63.93ms unsure 🔍
-5% - -0%
-3.09ms - +0.02ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 427 kB 317.10ms - 324.68ms - faster ✔
19% - 22%
76.50ms - 88.33ms
branch 443 kB 398.76ms - 407.85ms slower ❌
24% - 28%
76.50ms - 88.33ms
-

overlay permalink

Version Bytes Avg Time vs remote vs branch
npm latest 463 kB 99.66ms - 102.84ms - unsure 🔍
-3% - +1%
-3.17ms - +1.26ms
branch 459 kB 100.66ms - 103.75ms unsure 🔍
-1% - +3%
-1.26ms - +3.17ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 602 kB 851.87ms - 868.51ms - faster ✔
27% - 29%
318.04ms - 344.63ms
branch 613 kB 1181.15ms - 1201.90ms slower ❌
37% - 40%
318.04ms - 344.63ms
-

popover permalink

Version Bytes Avg Time vs remote vs branch
npm latest 373 kB 47.35ms - 49.09ms - unsure 🔍
-2% - +3%
-0.89ms - +1.40ms
branch 368 kB 47.22ms - 48.70ms unsure 🔍
-3% - +2%
-1.40ms - +0.89ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 459 kB 202.97ms - 207.11ms - unsure 🔍
-1% - +3%
-1.75ms - +5.12ms
branch 455 kB 200.61ms - 206.10ms unsure 🔍
-2% - +1%
-5.12ms - +1.75ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 680 kB 2058.19ms - 2064.72ms - faster ✔
0% - 1%
2.17ms - 11.56ms
branch 689 kB 2064.95ms - 2071.70ms slower ❌
0% - 1%
2.17ms - 11.56ms
-

tooltip permalink

Version Bytes Avg Time vs remote vs branch
npm latest 367 kB 47.08ms - 48.72ms - unsure 🔍
-3% - +3%
-1.55ms - +1.21ms
branch 363 kB 46.96ms - 49.18ms unsure 🔍
-3% - +3%
-1.21ms - +1.55ms
-
Firefox

action-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 476 kB 313.95ms - 339.17ms - unsure 🔍
-3% - +7%
-9.21ms - +21.25ms
branch 472 kB 312.00ms - 329.08ms unsure 🔍
-6% - +3%
-21.25ms - +9.21ms
-

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 748 kB 343.01ms - 356.19ms - faster ✔
8% - 15%
28.30ms - 58.54ms
branch 758 kB 379.41ms - 406.63ms slower ❌
8% - 17%
28.30ms - 58.54ms
-

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 499 kB 238.70ms - 258.38ms - unsure 🔍
-4% - +7%
-9.21ms - +17.01ms
branch 495 kB 235.97ms - 253.31ms unsure 🔍
-7% - +4%
-17.01ms - +9.21ms
-

illustrated-message permalink

Version Bytes Avg Time vs remote vs branch
npm latest 395 kB 122.38ms - 141.42ms - unsure 🔍
-10% - +9%
-13.18ms - +12.10ms
branch 391 kB 124.12ms - 140.76ms unsure 🔍
-9% - +10%
-12.10ms - +13.18ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 427 kB 453.49ms - 474.67ms - faster ✔
29% - 33%
192.90ms - 224.06ms
branch 443 kB 661.14ms - 683.98ms slower ❌
41% - 49%
192.90ms - 224.06ms
-

overlay permalink

Version Bytes Avg Time vs remote vs branch
npm latest 463 kB 160.16ms - 174.40ms - unsure 🔍
-5% - +8%
-8.70ms - +12.74ms
branch 459 kB 157.25ms - 173.27ms unsure 🔍
-8% - +5%
-12.74ms - +8.70ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 602 kB 893.94ms - 918.02ms - faster ✔
21% - 24%
247.53ms - 285.87ms
branch 613 kB 1157.76ms - 1187.60ms slower ❌
27% - 32%
247.53ms - 285.87ms
-

popover permalink

Version Bytes Avg Time vs remote vs branch
npm latest 373 kB 105.48ms - 125.96ms - unsure 🔍
-5% - +23%
-4.46ms - +23.34ms
branch 368 kB 96.89ms - 115.67ms unsure 🔍
-20% - +3%
-23.34ms - +4.46ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 459 kB 324.51ms - 344.73ms - unsure 🔍
-3% - +5%
-10.79ms - +16.47ms
branch 455 kB 322.64ms - 340.92ms unsure 🔍
-5% - +3%
-16.47ms - +10.79ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 680 kB 2146.18ms - 2156.62ms - unsure 🔍
-1% - +0%
-11.85ms - +2.85ms
branch 689 kB 2150.72ms - 2161.08ms unsure 🔍
-0% - +1%
-2.85ms - +11.85ms
-

tooltip permalink

Version Bytes Avg Time vs remote vs branch
npm latest 367 kB 79.82ms - 90.62ms - faster ✔
1% - 19%
0.31ms - 18.41ms
branch 363 kB 87.32ms - 101.84ms unsure 🔍
-0% - +22%
+0.31ms - +18.41ms
-

@Westbrook
Copy link
Collaborator

FYI: this did a weird thing: https://9905080f40f3bb038d2df931c70e6c4c--spectrum-web-components.netlify.app/review/#ActionMenuStories/selects.png

image

@najikahalsema najikahalsema changed the title menu: convert to core tokens feat(menu): convert to core tokens Jul 31, 2023
@najikahalsema najikahalsema marked this pull request as ready for review July 31, 2023 20:10
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.

https://dd8f58a27a69ec29ce5a86faac0d3141--spectrum-web-components.netlify.app/review/#SubmenuStories/submenu.png

"Submenu chevrons" are getting margin-inline-end from what I think is a rule meant for "collapsable chevrons" in the Spectrum CSS source. Can you confirm with their team that this is being applied appropriately.

Current:
image

Without the added margin:
image

packages/menu/src/spectrum-config.js Show resolved Hide resolved
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.

Looking real nice to me! Looking good to you?

Final practical steps: 🤞🏼

  • get a stable release from the CSS team
  • add "Sizes" stories to the Menu package
  • pass this.size down from Picker.ts into the <sp-menu> child in its shadow DOM
  • add a set of "Sizes Open" stories to Picker so that the above change is included in VRTs
  • (possible overkill, but nice to have for sure) add a set of "Sizes Open" stories to Action Menu and Split Button
  • update the VRT cache with the updated and new stories

Very exciting!!

@najikahalsema najikahalsema force-pushed the menu-core-tokens branch 5 times, most recently from 1999850 to 1a352cd Compare August 7, 2023 19:19
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.

LGTM!!! 🚀

@najikahalsema najikahalsema merged commit da43540 into main Aug 14, 2023
46 checks passed
@najikahalsema najikahalsema deleted the menu-core-tokens branch August 14, 2023 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants