-
Notifications
You must be signed in to change notification settings - Fork 201
fix(swatch): prepares swatch for new stories #2933
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(swatch): prepares swatch for new stories #2933
Conversation
|
File metricsSummaryTotal size: 4.63 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsswatch
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-2933--spectrum-css.netlify.app |
- adds the background property for .is-image class for gradient and/or image fills - adds z-index for mixedValueIcon so it sits "on top" of background color
1819b5d
to
c30f8a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for walking me through the changes with your validation instructions! This looks pretty good but I had a few follow-up questions/comments:
- I can't use the controls to switch the gradient on/off or adjust the gradient colors because they weren't added as
argTypes
. The stories also display in the sidebar, and I'm not sure that's necessarily what we want to do. But these are temporary changes added in the validation anyway, so I'm not sure if that's relevant here, but I wanted to note it anyway in case the same code is being used in a different PR. - Everything seems to work as expected, except that I don't see the opacity checkerboard in the background if I set gradient stops to transparent. Is there a reason the
background
property isn't set on the::before
element since that's where thebackground-color
was set before?
@@ -150,6 +150,11 @@ | |||
/* Swatch fill: Image, Gradient, SVG */ | |||
&.is-image { | |||
.spectrum-Swatch-fill { | |||
/* Undefined variables allows custom stylesheet or JS to pass the value to this element */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* Undefined variables allows custom stylesheet or JS to pass the value to this element */ | |
/* Undefined variables allow custom stylesheet or JS to pass the value to this element */ |
During review, we found I was misusing the |
Description
As documentation migration work occurred on #2925, it was noted that adding certain swatch and/or swatch group stories would not be functional without additional CSS. For stories found on the swatch docs page, like image and gradient, a
background
property would need to be added to the shared.is-image
class. For the mixed value variant, adding az-index: 2
(mirroring the disabled icon variant) should allow an icon to sit "on top" of the swatch. Both approaches mimic CSS strategies in place for other classes and/or stories.Jira CSS-854
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
git checkout 538be42a54b396fc767f461d1c8ea5dbf945b2a7
to view the branch with temporary changes to the template (the changes you see in this commit are purposely reverted for the purposes of validation, and should be added in docs(swatch,swatchgroup,table,tabs): docs migrations to storybook #2925)swatch/stories/swatch.stories.js
, add the following code:gradient
value, theimageUrl
, or turn off theisMixedValue
arg to verify the new CSS changes are respondingswatch/index.css
, comment out the following lines (approx. lines 154-156) for.spectrum-Swatch { &.is-image { .spectrum-Swatch-fill
opacitycheckboard
componentbackground
property.spectrum-Swatch-mixedValueIcon
(approx. line 292)z-index
propertygit switch -
in your terminal to return to the tip of this branch and discard your changes. The temporary stories are no long appearing on the docs pageRegression testing
Validate:
Screenshots
To-do list