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(opacitycheckerboard): address concerns about inclusion order #2382

Merged
merged 5 commits into from
Jan 22, 2024

Conversation

jenndiaz
Copy link
Contributor

@jenndiaz jenndiaz commented Dec 20, 2023

Description

Addresses: #2139

"With both the spectrum-ColorHandle and the spectrum-OpacityCheckerboard classes applied to the host element the rules within those selectors will race depending on how CSS is deployed in a consuming application. Specifically, the size rules for .spectrum-OpacityCheckerboard
Can overwrite those of .spectrum-ColorHandle:"

Changes:

  • removes "inline-size": "100%", "block-size": "100%", since the background should be used with a component with these properties already set, this is not needed with in this classes CSS
  • move from components in storybook into utilities
  • improve docs for component and update storybook

These changes as well as the SWC documentation improvements merged in with SWC PR 3651 addresses concerns around inclusion order when using the opacity checkerboard.

Alternative approach to now closed PR #2350

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

tested by @mdt2

On the docs site:

  • opacity checkerboard displayed with no changes
  • no changes to opacity checkerboard within Colorhandle, Colorslider, Swatch, or Thumbnail

In storybook:

  • no changes to opacity checkerboard within Colorhandle, Colorslider, Swatch, or Thumbnail
  • opacity checkerboard moved to Utilities folder in the side nav

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 20, 2023

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

Copy link
Contributor

github-actions bot commented Dec 20, 2023

File metrics

Summary

Total size: 3.91 MB*
Total change (Δ): ⬇ 0.11 KB (-0.00%)
Table reports on changes to a package's main file. Other changes can be found in the collapsed "Details" below.

Package Size Δ
opacitycheckerboard 1.61 KB ⬇ 0.04 KB
Details

opacitycheckerboard

File Head Base Δ
index-base.css 1.61 KB 1.64 KB ⬇ 0.04 KB (-2.20%)
index-vars.css 1.61 KB 1.64 KB ⬇ 0.04 KB (-2.20%)
index.css 1.61 KB 1.64 KB ⬇ 0.04 KB (-2.20%)
mods.json 0.18 KB 0.18 KB -
* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@jenndiaz jenndiaz marked this pull request as ready for review December 21, 2023 23:09
Copy link
Collaborator

@mdt2 mdt2 left a comment

Choose a reason for hiding this comment

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

Seeing a couple wonkies in the components that use opacity checkerboard, thinking it's related to the custom styles at least for the storybook ones:

  • Swatch in the docs site***: swatch-weird
  • Color handle in storybook: colorhandle-storybook
  • Color slider in storybook: colorslider-storybook

I didn't check them all in Storybook since it seemed possible that the issue across components was related.

*** Edit: we chatted and this is a small change in the swatch.yml that will be updated in a separate PR.

@jenndiaz
Copy link
Contributor Author

jenndiaz commented Jan 2, 2024

Seeing a couple wonkies in the components that use opacity checkerboard, thinking it's related to the custom styles at least for the storybook ones:

ah, yes you're right was an issue with customStyles, the 100% for inline and block size needed to be applied to just the stories for displayed opacity checkerboard not within a component. I have pushed up a fix for this.

Copy link
Collaborator

@mdt2 mdt2 left a comment

Choose a reason for hiding this comment

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

Code and example sites look good to me based on the validation steps. Curious to hear what thoughts Westbrook has.

Copy link
Collaborator

@jawinn jawinn left a comment

Choose a reason for hiding this comment

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

These changes look good to me. I like that this is organized into "Utilities" now in Storybook.

Copy link
Collaborator

@castastrophe castastrophe left a comment

Choose a reason for hiding this comment

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

Let's revert the category update for now but otherwise this is looking great.

@@ -5,7 +5,7 @@ import { styleMap } from "lit/directives/style-map.js";
import { Template } from "./template";

export default {
title: "Components/Opacity checkerboard",
title: "Utilities/Opacity checkerboard",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good clarification but let's hold off creating new headings yet. I'd like to go through our whole list of components and determine what others can fall under this same category.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted for now!

I agree that it would be useful to look at all components and see what categories make sense.

examples:
- id: opacity-checkerboard
name: Opacity Checkerboard
name: Opacity checkerboard
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for updating this to align with the style guide!

@jenndiaz jenndiaz force-pushed the jenndiaz/css-587-opacitycheckerboard-inclusion-order branch from c934e19 to 43bbf04 Compare January 22, 2024 16:11
@pfulton pfulton merged commit 8d44673 into main Jan 22, 2024
11 checks passed
@pfulton pfulton deleted the jenndiaz/css-587-opacitycheckerboard-inclusion-order branch January 22, 2024 17:04
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.

5 participants