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

refactor(opacity checkerboard)!: refactor from component into commons #2350

Closed
wants to merge 9 commits into from

Conversation

jenndiaz
Copy link
Contributor

@jenndiaz jenndiaz commented Dec 7, 2023

Description

BREAKING CHANGE - would require removing a component

Refactors Opacity Checkerboard from a component to be with in components/commons/opacity-checkerboard.css this addresses concerns around inclusion order overwriting css values (#2139) and simplifies opacity checkerboard.

Alternative solution to the GitHub issue: move css unnecessary to implement the background pattern, specifically inline-size, block-size from the index.css and add to inline styling for the component docs site and storybook.

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Docs Site
Opacity Checkerboard should appear correctly on:

  • colorhandle
  • mods with in opacity checkerboard still work as expected (look at --mod-opacity-checkerboard-position with in colorhandle)
  • colorslider
  • thumbnail
  • swatch

Storybook
Opacity Checkerboard should appear correctly on:

  • colorhandle
  • colorslider
  • thumbnail
  • swatch

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

Copy link
Contributor

github-actions bot commented Dec 11, 2023

🚀 Deployed on https://pr-2350--spectrum-css.netlify.app

Copy link
Contributor

github-actions bot commented Dec 11, 2023

File metrics

Summary

Total size: 4.01 MB*
Total change (Δ): ⬇ 4.50 KB (0.11%)

Package Size Δ
colorhandle 16.66 KB ⬇ 2.67 KB
colorslider 14.69 KB ⬇ 2.66 KB
swatch 35.85 KB ⬇ 2.69 KB
thumbnail 26.06 KB ⬇ 2.72 KB
Details

colorhandle

File Size Base Δ
Total 19.33 KB 16.66 KB ⬇ 2.67 KB (13.84%)
index-base.css 5.86 KB 4.97 KB ⬇ 0.91 KB (15.21%)
index-theme.css 0.59 KB 0.59 KB - (no change)
index-vars.css 5.86 KB 4.97 KB ⬇ 0.91 KB (15.21%)
index.css 5.86 KB 4.97 KB ⬇ 0.91 KB (15.21%)
mods.json 0.60 KB 0.60 KB - (no change)
themes/express.css 0.59 KB 0.59 KB - (no change)
themes/spectrum.css < 0.01 KB < 0.01 KB - (no change)

colorslider

File Size Base Δ
Total 17.36 KB 14.69 KB ⬇ 2.66 KB (15.34%)
index-base.css 5.20 KB 4.31 KB ⬇ 0.91 KB (17.07%)
index-theme.css 0.59 KB 0.59 KB - (no change)
index-vars.css 5.20 KB 4.31 KB ⬇ 0.91 KB (17.07%)
index.css 5.20 KB 4.31 KB ⬇ 0.91 KB (17.07%)
mods.json 0.61 KB 0.61 KB - (no change)
themes/express.css 0.59 KB 0.59 KB - (no change)
themes/spectrum.css < 0.01 KB < 0.01 KB - (no change)

swatch

File Size Base Δ
Total 38.54 KB 35.85 KB ⬇ 2.69 KB (6.99%)
index-base.css 12.24 KB 11.34 KB ⬇ 0.92 KB (7.33%)
index-theme.css 0.59 KB 0.59 KB - (no change)
index-vars.css 12.24 KB 11.34 KB ⬇ 0.92 KB (7.33%)
index.css 12.24 KB 11.34 KB ⬇ 0.92 KB (7.33%)
mods.json 0.69 KB 0.69 KB - (no change)
themes/express.css 0.59 KB 0.59 KB - (no change)
themes/spectrum.css < 0.01 KB < 0.01 KB - (no change)

thumbnail

File Size Base Δ
Total 28.79 KB 26.06 KB ⬇ 2.72 KB (9.46%)
index-base.css 9.39 KB 8.49 KB ⬇ 0.93 KB (9.67%)
index-theme.css < 0.01 KB < 0.01 KB - (no change)
index-vars.css 9.39 KB 8.49 KB ⬇ 0.93 KB (9.67%)
index.css 9.39 KB 8.49 KB ⬇ 0.93 KB (9.67%)
mods.json 0.62 KB 0.62 KB - (no change)
themes/express.css < 0.01 KB < 0.01 KB - (no change)
themes/spectrum.css < 0.01 KB < 0.01 KB - (no change)

* An ASCII character in UTF-8 is 8 bits or 1 byte.

@jenndiaz
Copy link
Contributor Author

jenndiaz commented Dec 11, 2023

mod file is currently not generating mods for opacity checkerboard, this is being addressed in #2322

@@ -105,7 +120,7 @@

/* Visually hidden "range" input field */
.spectrum-ColorSlider-slider {
opacity: 0;
opacity: 0%;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

style lint wants opacities in %

@jenndiaz jenndiaz marked this pull request as ready for review December 11, 2023 22:18
@pfulton
Copy link
Collaborator

pfulton commented Dec 13, 2023

@Westbrook I'd be curious to get your thoughts on this refactor. I know this was implemented differently in SWC (not a component) vs. how we have it in our repo (a "component"). What level of churn (if any) would this cause for you all?

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.

I'm not sure the right path for continued consumption of this approach in SWC without seeing the actual published code. For us to ship it as we have it needs to be staic somewhere in the output, separate from the consuming patterns.

Things that I do know:

  • using CSS preprocessing means that there are multiple definitions of the CSS that can be shipped to users, rather than a single copy.
  • we do have users that leverage the Opacity Checkerboard outside of the Spectrum elements
  • the stand alone class was a useful way to support normalization across the consumption of this in both Spectrum patterns and Adobe apps and features at large.

@jenndiaz
Copy link
Contributor Author

I'm not sure the right path for continued consumption of this approach in SWC without seeing the actual published code. For us to ship it as we have it needs to be staic somewhere in the output, separate from the consuming patterns.

Things that I do know:

  • using CSS preprocessing means that there are multiple definitions of the CSS that can be shipped to users, rather than a single copy.
  • we do have users that leverage the Opacity Checkerboard outside of the Spectrum elements
  • the stand alone class was a useful way to support normalization across the consumption of this in both Spectrum patterns and Adobe apps and features at large.

@pfulton and @Westbrook

I think based on the fact that people are using this outside of Spectrum elements steers me towards a more simple approach. I'll look at simplifying the CSS if I can and documenting inclusion order more clearly.

I think this could be a good option to be more of "utility class" like we are looking at with background layers.

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.

3 participants