-
Notifications
You must be signed in to change notification settings - Fork 195
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(colorarea): S2 migration #3388
Conversation
🦋 Changeset detectedLatest commit: dd5c702 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
b98d4a5
to
8a90157
Compare
🚀 Deployed on https://pr-3388--spectrum-css.netlify.app |
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.
Detailscolorarea
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
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.
This one seems pretty straightforward. I did come away with some questions after reviewing.
- The designs make a point to include the color loupe in the list of components for color area. Should we add some documentation regarding that? I'm not sure we need to add the color loupe to this template, but I was thinking at least a blurb about "implementations can use the color loupe to..." I don't think there's anything on the
main
about color loupe with this color area component, but it is mentioned in the Spectrum guidelines: https://spectrum.adobe.com/page/color-area/#Color-loupe-on-down/touch-state Or maybe we link to the color handle docs page, where it talks about color loupe? https://opensource.adobe.com/spectrum-css/preview/index.html?path=/docs/components-color-handle--docs Feel free to get other opinions on this! - Something I just thought of- I wonder if we should move the
forced-colors
query. I would have to check with Rise and Josh (or dig through old PRs), but I think when we were doing these components before, we were trying to move the forced-colors block to the bottom of the CSS file to work with the cascade.
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.
Looks like everything is connecting to the right tokens, there are some duplicated custom properties being used for the border-radii that we'd probably want to sort out though.
Comparing this branch with main
, I'm noticing that the component sizes are different, which is fine, but zooming in on the smaller size in this branch I noticed that there's a gap in the corners between the border and the gradient:
This happens in main
too, but I'm wondering if that's a thing we can fix for S2. We'd probably want to set the border-radius
in .spectrum-ColorArea-gradient
to something like calc(var(--spectrum-colorarea-border-radius) - var(--spectrum-colorarea-border-width))
and maybe leave a comment explaining it (I didn't include the mods here but you'd want to add them in too).
8a90157
to
a79755d
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.
Looks great! I think the changeset needs updating to match the changes that have been made here, but otherwise I think this is good to go!
.changeset/spotty-onions-study.md
Outdated
|
||
# colorarea S2 migration | ||
|
||
This change migrates the `colorarea` component to S2. It updates the `--spectrum-colorarea-border-radius` token to `--spectrum-colorarea-border-rounding`. Additionally, it leverages updated tokens and the `rgba(...)` color function, replacing the existing `TODO` and hardcoded values. |
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.
Do we need "It updates the --spectrum-colorarea-border-radius
token to --spectrum-colorarea-border-rounding
."? We took out the --spectrum-colorarea-border-rounding
custom prop, if I'm remembering correctly?
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.
Just updated this — thanks so much for catching it! ✨
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 making all of the changes. Looks like everything was take care of from my perspective and things are looking great. I think the only thing we should do is update that changeset like Rise mentioned, but I'm going to approve. Nice work!
* feat(colorarea): s2 migration * feat(colorarea): remove redundant property * feat(colorarea): clarify color + rgb implementation * feat(colorarea): fix gradient border-radius * feat(colorarea): move forced-colors media query * feat(colorarea): update changeset
* feat(colorarea): s2 migration * feat(colorarea): remove redundant property * feat(colorarea): clarify color + rgb implementation * feat(colorarea): fix gradient border-radius * feat(colorarea): move forced-colors media query * feat(colorarea): update changeset
* feat(colorarea): s2 migration * feat(colorarea): remove redundant property * feat(colorarea): clarify color + rgb implementation * feat(colorarea): fix gradient border-radius * feat(colorarea): move forced-colors media query * feat(colorarea): update changeset
Description
CSS-1032
This change migrates the
colorarea
component to S2. It updates the--spectrum-colorarea-border-radius
token to--spectrum-colorarea-border-rounding
. Additionally, it leverages updated tokens and thergba(...)
color function, replacing the existingTODO
and hardcoded values.Validation steps
colorarea
component.To-do list