spectrum-web-components icon indicating copy to clipboard operation
spectrum-web-components copied to clipboard

refactor(badge): use core tokens

Open lunarfusion opened this issue 3 years ago β€’ 2 comments

This updates the Badge component to use core tokens

Uses beta release. Hold for full release.

Description

Related issue(s)

Motivation and context

How has this been tested?

  • [ ] Test case 1
    1. Go here
    2. Do this
  • [ ] Test case 2
    1. Go here
    2. Do this

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.

lunarfusion avatar Sep 08 '22 17:09 lunarfusion

It appears that all the failures are likely due to changed markup that adds a "spectrum-Badge-label" element to control the spacing required by the addition of the icon variant.

Screen Shot 2022-09-12 at 10 32 39 AM

The markup of web-components will need to be updated to reflect the correct styles. Please advise if I should make any changes.

@Westbrook

lunarfusion avatar Sep 12 '22 15:09 lunarfusion

There are a lot of DOM changes here, yes. @pfulton this makes the Badge even more like the Action Button, I know we don't usually share code this way, but should we share code here?

With the differences, would it be better for us to take this one on, or would it make sense to pair a little on it or?

Westbrook avatar Sep 26 '22 19:09 Westbrook

There are a lot of DOM changes here, yes. @pfulton this makes the Badge even more like the Action Button, I know we don't usually share code this way, but should we share code here?

😬 I'm not sure! My gut reaction is no, don't share - I'm more worried about how consumers would understand the differences between the two than I am about the code side of it. Do you think it'd be difficult to advise folks on what to use and when (especially if there would come a time when folks would need to customize either of these?).

That is, unless you're talking about having some "base" level component that both of these would inherit from. Maybe then I could see some sharing.

With the differences, would it be better for us to take this one on, or would it make sense to pair a little on it or?

Yeah, I think given our workload, we might need to rely on you all for the integration work. We're happy to support/fix any bugs with the CSS, but for this one, we might need y'all to carry it across the finish line.

pfulton avatar Sep 29 '22 20:09 pfulton

Tachometer results

Chrome

action-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 244 kB 32.83ms - 33.13ms - unsure πŸ”
-1% - +0%
-0.36ms - +0.09ms
branch 244 kB 32.96ms - 33.28ms unsure πŸ”
-0% - +1%
-0.09ms - +0.36ms
-

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 612 kB 219.01ms - 220.84ms - unsure πŸ”
-1% - +0%
-2.70ms - +0.83ms
branch 610 kB 219.35ms - 222.38ms unsure πŸ”
-0% - +1%
-0.83ms - +2.70ms
-

badge permalink

Version Bytes Avg Time vs remote vs branch
npm latest 275 kB 40.61ms - 40.87ms - faster βœ”
13% - 13%
5.89ms - 6.30ms
branch 271 kB 46.68ms - 46.99ms slower ❌
14% - 15%
5.89ms - 6.30ms
-

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 392 kB 86.71ms - 88.10ms - unsure πŸ”
-1% - +2%
-0.71ms - +1.36ms
branch 391 kB 86.31ms - 87.84ms unsure πŸ”
-2% - +1%
-1.36ms - +0.71ms
-

illustrated-message permalink

Version Bytes Avg Time vs remote vs branch
npm latest 293 kB 40.64ms - 40.98ms - unsure πŸ”
-0% - +1%
-0.13ms - +0.35ms
branch 293 kB 40.53ms - 40.87ms unsure πŸ”
-1% - +0%
-0.35ms - +0.13ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 311 kB 247.18ms - 249.16ms - unsure πŸ”
-1% - +0%
-2.99ms - +0.34ms
branch 311 kB 248.16ms - 250.84ms unsure πŸ”
-0% - +1%
-0.34ms - +2.99ms
-

overlay permalink

Version Bytes Avg Time vs remote vs branch
npm latest 348 kB 75.02ms - 75.45ms - unsure πŸ”
-1% - +0%
-0.63ms - +0.37ms
branch 348 kB 74.91ms - 75.82ms unsure πŸ”
-0% - +1%
-0.37ms - +0.63ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 456 kB 797.72ms - 806.74ms - unsure πŸ”
-3% - +0%
-22.13ms - +3.13ms
branch 454 kB 799.93ms - 823.53ms unsure πŸ”
-0% - +3%
-3.13ms - +22.13ms
-

popover permalink

Version Bytes Avg Time vs remote vs branch
npm latest 241 kB 30.12ms - 30.42ms - unsure πŸ”
-0% - +1%
-0.13ms - +0.23ms
branch 241 kB 30.13ms - 30.31ms unsure πŸ”
-1% - +0%
-0.23ms - +0.13ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 341 kB 148.01ms - 149.06ms - unsure πŸ”
-1% - +1%
-1.09ms - +0.99ms
branch 340 kB 147.69ms - 149.49ms unsure πŸ”
-1% - +1%
-0.99ms - +1.09ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 548 kB 2028.15ms - 2031.03ms - unsure πŸ”
-0% - +0%
-1.90ms - +2.28ms
branch 547 kB 2027.88ms - 2030.93ms unsure πŸ”
-0% - +0%
-2.28ms - +1.90ms
-

tooltip permalink

Version Bytes Avg Time vs remote vs branch
npm latest 248 kB 30.22ms - 30.43ms - unsure πŸ”
-1% - +0%
-0.18ms - +0.13ms
branch 247 kB 30.23ms - 30.46ms unsure πŸ”
-0% - +1%
-0.13ms - +0.18ms
-
Firefox

action-bar permalink

Version Bytes Avg Time vs remote vs branch
npm latest 244 kB 113.51ms - 127.05ms - unsure πŸ”
-5% - +11%
-5.89ms - +13.17ms
branch 244 kB 109.94ms - 123.34ms unsure πŸ”
-11% - +5%
-13.17ms - +5.89ms
-

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 612 kB 422.57ms - 437.07ms - unsure πŸ”
-3% - +2%
-12.67ms - +8.99ms
branch 610 kB 423.62ms - 439.70ms unsure πŸ”
-2% - +3%
-8.99ms - +12.67ms
-

badge permalink

Version Bytes Avg Time vs remote vs branch
npm latest 275 kB 118.32ms - 129.84ms - unsure πŸ”
-9% - +1%
-12.32ms - +1.68ms
branch 271 kB 125.43ms - 133.37ms unsure πŸ”
-2% - +10%
-1.68ms - +12.32ms
-

card permalink

Version Bytes Avg Time vs remote vs branch
npm latest 392 kB 185.04ms - 200.72ms - unsure πŸ”
-7% - +5%
-12.71ms - +9.27ms
branch 391 kB 186.90ms - 202.30ms unsure πŸ”
-5% - +7%
-9.27ms - +12.71ms
-

illustrated-message permalink

Version Bytes Avg Time vs remote vs branch
npm latest 293 kB 93.95ms - 103.29ms - unsure πŸ”
-11% - +4%
-11.39ms - +3.99ms
branch 293 kB 96.21ms - 108.43ms unsure πŸ”
-4% - +12%
-3.99ms - +11.39ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 311 kB 542.29ms - 555.59ms - faster βœ”
1% - 6%
7.56ms - 33.04ms
branch 311 kB 558.38ms - 580.10ms slower ❌
1% - 6%
7.56ms - 33.04ms
-

overlay permalink

Version Bytes Avg Time vs remote vs branch
npm latest 348 kB 168.53ms - 177.19ms - unsure πŸ”
-8% - +1%
-14.64ms - +1.28ms
branch 348 kB 172.86ms - 186.22ms unsure πŸ”
-1% - +9%
-1.28ms - +14.64ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 456 kB 1171.32ms - 1184.72ms - unsure πŸ”
-1% - +1%
-10.87ms - +7.47ms
branch 454 kB 1173.45ms - 1185.99ms unsure πŸ”
-1% - +1%
-7.47ms - +10.87ms
-

popover permalink

Version Bytes Avg Time vs remote vs branch
npm latest 241 kB 102.17ms - 120.63ms - unsure πŸ”
-12% - +10%
-13.81ms - +10.93ms
branch 241 kB 104.61ms - 121.07ms unsure πŸ”
-10% - +12%
-10.93ms - +13.81ms
-

slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 341 kB 353.56ms - 364.60ms - unsure πŸ”
-3% - +2%
-9.76ms - +8.04ms
branch 340 kB 352.95ms - 366.93ms unsure πŸ”
-2% - +3%
-8.04ms - +9.76ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 548 kB 2172.35ms - 2183.01ms - unsure πŸ”
-1% - +0%
-12.66ms - +5.58ms
branch 547 kB 2173.82ms - 2188.62ms unsure πŸ”
-0% - +1%
-5.58ms - +12.66ms
-

tooltip permalink

Version Bytes Avg Time vs remote vs branch
npm latest 248 kB 81.02ms - 89.50ms - unsure πŸ”
-8% - +8%
-6.44ms - +6.60ms
branch 247 kB 80.22ms - 90.14ms unsure πŸ”
-8% - +8%
-6.60ms - +6.44ms
-

github-actions[bot] avatar Oct 26 '22 12:10 github-actions[bot]

@hunterloftis you helped us bring this to life to begin with, and there are a bunch of changes included to adopted the new CSS dependency, so I was hoping you could take a closer look and make sure this aligns with anything that might remain in your head regarding the implementation of this.

Westbrook avatar Oct 26 '22 14:10 Westbrook

Sorry, I missed this GH ping @Westbrook.

Re: the similarity w/ action button, I think the guidance for users boils down to: Is this an interactive component?

  • Yes: Action Button
  • No: Badge

@lunarfusion where can I learn more about the 'fixed' variant being deprecated? That variant is used in several places in production & doesn't appear deprecated in the design spec.

hunterloftis avatar Nov 02 '22 14:11 hunterloftis

The fixed version itself isn’t deprecated, the values for that attribute have been updated to be logical property based so inline-start rather than left, which is a choose of mine.

Westbrook avatar Nov 02 '22 14:11 Westbrook

@Westbrook I must be missing something; the updated values look like colors? I'll take a closer look later.

hunterloftis avatar Nov 02 '22 15:11 hunterloftis