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: opacity checkerboard inclusion order #3651

Merged
merged 2 commits into from Sep 14, 2023
Merged

Conversation

jawinn
Copy link
Collaborator

@jawinn jawinn commented Sep 13, 2023

Description

  • Updated docs around opacity-checkerboard inclusion order and fixed a path that was wrong there
  • Updated order of styles in only component I found where the checkerboard styles were after the component styles

Related to some discussion around an open issue around inclusion order in Spectrum CSS, and thoughts around doc updates when I was integrating opacity-checkerboard with ColorSlider.

How has this been tested?

Validation Steps

  • Make sure ColorLoupe visuals have not changed
  • Review updated docs for opacity-checkerboard

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.

Update README for the opacity-checkerboard class, to explain that this
class should be included before component styles. And updates a wrong
import path.
Make sure opacity checkerboard styles are included before the component
styles, to make sure any component styles with the same specificity take
precendence.
@github-actions
Copy link

github-actions bot commented Sep 13, 2023

Tachometer results

Chrome

color-area permalink

Version Bytes Avg Time vs remote vs branch
npm latest 445 kB 205.42ms - 206.23ms - slower ❌
1% - 2%
1.96ms - 3.09ms
branch 441 kB 202.90ms - 203.69ms faster ✔
1% - 2%
1.96ms - 3.09ms
-

color-handle permalink

Version Bytes Avg Time vs remote vs branch
npm latest 363 kB 63.90ms - 64.36ms - slower ❌
4% - 5%
2.51ms - 3.09ms
branch 361 kB 61.16ms - 61.50ms faster ✔
4% - 5%
2.51ms - 3.09ms
-

color-loupe permalink

Version Bytes Avg Time vs remote vs branch
npm latest 355 kB 43.78ms - 44.08ms - slower ❌
7% - 8%
2.91ms - 3.27ms
branch 352 kB 40.73ms - 40.93ms faster ✔
7% - 7%
2.91ms - 3.27ms
-

color-slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 446 kB 170.62ms - 172.89ms - unsure 🔍
-0% - +1%
-0.38ms - +2.30ms
branch 443 kB 170.09ms - 171.51ms unsure 🔍
-1% - +0%
-2.30ms - +0.38ms
-

color-wheel permalink

Version Bytes Avg Time vs remote vs branch
npm latest 448 kB 171.47ms - 177.86ms - slower ❌
0% - 4%
0.62ms - 7.05ms
branch 444 kB 170.48ms - 171.18ms faster ✔
0% - 4%
0.62ms - 7.05ms
-

swatch permalink

Version Bytes Avg Time vs remote vs branch
npm latest 396 kB 39.08ms - 39.48ms - faster ✔
0% - 2%
0.03ms - 0.63ms
branch 392 kB 39.40ms - 39.82ms slower ❌
0% - 2%
0.03ms - 0.63ms
-

thumbnail permalink

Version Bytes Avg Time vs remote vs branch
npm latest 754 kB 50.00ms - 50.39ms - unsure 🔍
-1% - +1%
-0.29ms - +0.35ms
branch 747 kB 49.91ms - 50.42ms unsure 🔍
-1% - +1%
-0.35ms - +0.29ms
-
Firefox

color-area permalink

Version Bytes Avg Time vs remote vs branch
npm latest 445 kB 458.27ms - 487.73ms - faster ✔
4% - 13%
18.77ms - 65.63ms
branch 441 kB 496.99ms - 533.41ms slower ❌
4% - 14%
18.77ms - 65.63ms
-

color-handle permalink

Version Bytes Avg Time vs remote vs branch
npm latest 363 kB 209.24ms - 238.76ms - unsure 🔍
-12% - +5%
-29.16ms - +12.52ms
branch 361 kB 217.60ms - 247.04ms unsure 🔍
-6% - +13%
-12.52ms - +29.16ms
-

color-loupe permalink

Version Bytes Avg Time vs remote vs branch
npm latest 355 kB 150.14ms - 174.66ms - unsure 🔍
-16% - +2%
-29.13ms - +4.13ms
branch 352 kB 163.66ms - 186.14ms unsure 🔍
-3% - +18%
-4.13ms - +29.13ms
-

color-slider permalink

Version Bytes Avg Time vs remote vs branch
npm latest 446 kB 443.39ms - 469.33ms - faster ✔
3% - 12%
11.26ms - 59.26ms
branch 443 kB 471.42ms - 511.82ms slower ❌
2% - 13%
11.26ms - 59.26ms
-

color-wheel permalink

Version Bytes Avg Time vs remote vs branch
npm latest 448 kB 412.20ms - 434.80ms - faster ✔
8% - 16%
37.30ms - 76.90ms
branch 444 kB 464.34ms - 496.86ms slower ❌
9% - 18%
37.30ms - 76.90ms
-

swatch permalink

Version Bytes Avg Time vs remote vs branch
npm latest 396 kB 144.18ms - 166.90ms - unsure 🔍
-14% - +7%
-23.12ms - +11.60ms
branch 392 kB 148.17ms - 174.43ms unsure 🔍
-8% - +15%
-11.60ms - +23.12ms
-

thumbnail permalink

Version Bytes Avg Time vs remote vs branch
npm latest 754 kB 159.80ms - 184.44ms - unsure 🔍
-16% - +2%
-29.35ms - +4.19ms
branch 747 kB 173.32ms - 196.08ms unsure 🔍
-3% - +17%
-4.19ms - +29.35ms
-

@jawinn jawinn marked this pull request as ready for review September 13, 2023 21:32
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.

These notes don't actually apply in the case of the Color Loupe, but normalization will lead to better results long term. Thanks for adding these!

@Westbrook Westbrook merged commit 4f417dc into main Sep 14, 2023
47 checks passed
@Westbrook Westbrook deleted the checkerboard-inclusion-order branch September 14, 2023 11:41
TarunAdobe pushed a commit that referenced this pull request Sep 20, 2023
* docs(opacity-checkerboard): update docs around inclusion order

Update README for the opacity-checkerboard class, to explain that this
class should be included before component styles. And updates a wrong
import path.

* fix(color-loupe): adjust inclusion order of opacity-checkerboard

Make sure opacity checkerboard styles are included before the component
styles, to make sure any component styles with the same specificity take
precendence.
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