-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat(thumbnail-opacity-checkboard): S2 migration #3394
feat(thumbnail-opacity-checkboard): S2 migration #3394
Conversation
🦋 Changeset detectedLatest commit: cdeaa8f The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
File metricsSummaryTotal size: 4.29 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsopacitycheckerboard
thumbnail
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-3394--spectrum-css.netlify.app |
87bbb9c
to
b05d5ed
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.
Nice work. I did have a question about where you might think that thumbnail-specific opacity checkerboard token should go. And I left a real weird video I took in Chrome of the checkerboard changing the square sizes. 🤷♀️
94bc2dd
to
57cfb75
Compare
57cfb75
to
1e7042e
Compare
4b512aa
to
04a0bca
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.
There is a quick typo in the changeset, and we just noticed the border token was missing. I checked out a few of the other components that use opacity checkerboard (color slider, color handle, swatch) and I think things are looking good! Most of the opacity checkerboard instances have the 8px squares, but the thumbnails have the 4px.
04a0bca
to
6577d13
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.
Phew- glad we caught the border on the thumbnail! A couple of remaining thoughts:
- If it were me, I'd probably do a
patch
I think (or maybeminor
🤷♀️ ) for the thumbnail, instead of major. We just fixed the token name, so I don't think we introduced any breaking changes. - I'd also probably mention that fix in the changeset itself.
Nice work!
.spectrum-Thumbnail & { | ||
--spectrum-opacity-checkerboard-size: var(--spectrum-thumbnail-opacity-checkerboard-square-size); | ||
} |
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.
I think this adjustment belongs within the Thumbnail component CSS instead of the Opacity Checkerboard CSS. SWC would likely have trouble converting this as well.
Description
CSS-1056
This is a follow up to CSS-1023, implementing the
thumbnail-opacity-checkerboard-square-size
that is specific to thethumbnail
component but targeted at theopacity-checkerboard
.Adds thumbnail specific
--spectrum-opacity-checkerboard-size
token. This impacts the display of theopacity-checkerboard
when used within thethumbnail
component.Validation steps
thumbnail
component.Regression testing
Validate:
To-do list