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

feat(colorarea): S2 migration #3388

Merged
merged 6 commits into from
Nov 15, 2024
Merged

Conversation

cdransf
Copy link
Member

@cdransf cdransf commented Nov 11, 2024

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 the rgba(...) color function, replacing the existing TODO and hardcoded values.

Validation steps

  1. Run Storybook locally or open the link for the PR.
  2. Navigate to the colorarea component.
  3. Verify that no regressions are visible.

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. ✨

@cdransf cdransf added size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review labels Nov 11, 2024
@cdransf cdransf self-assigned this Nov 11, 2024
Copy link

changeset-bot bot commented Nov 11, 2024

🦋 Changeset detected

Latest commit: dd5c702

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@spectrum-css/colorarea Major
@spectrum-css/colorwheel Major

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

@cdransf cdransf changed the base branch from main to spectrum-two November 11, 2024 19:18
@cdransf cdransf changed the title feat(colorarea): s2 migration feat(colorarea): S2 migration Nov 11, 2024
@cdransf cdransf force-pushed the cdransf/s2-color-area-migration branch from b98d4a5 to 8a90157 Compare November 12, 2024 19:27
Copy link
Contributor

github-actions bot commented Nov 12, 2024

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

Copy link
Contributor

github-actions bot commented Nov 12, 2024

File metrics

Summary

Total size: 4.29 MB*
Total change (Δ): ⬆ 0.73 KB (0.02%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
colorarea 3.66 KB ⬆ 0.20 KB

Details

colorarea

File Head Base Δ
index-base.css 3.66 KB 3.46 KB ⬆ 0.20 KB (5.73%)
index-vars.css 3.66 KB 3.46 KB ⬆ 0.20 KB (5.73%)
index.css 3.66 KB 3.46 KB ⬆ 0.20 KB (5.73%)
metadata.json 1.69 KB 1.57 KB ⬆ 0.12 KB (7.53%)
* 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.

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a 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.

components/colorarea/index.css Show resolved Hide resolved
Copy link
Collaborator

@rise-erpelding rise-erpelding left a 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:
image

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).

components/colorarea/index.css Outdated Show resolved Hide resolved
@cdransf cdransf force-pushed the cdransf/s2-color-area-migration branch from 8a90157 to a79755d Compare November 14, 2024 00:19
@cdransf
Copy link
Member Author

cdransf commented Nov 14, 2024

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: image

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).

I'd noticed that too but I wasn't sure how best to approach it — thanks so much for clarifying! Just pushed up a fix for that.

Screenshot 2024-11-13 at 4 29 17 PM

Copy link
Collaborator

@rise-erpelding rise-erpelding left a 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!


# 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.
Copy link
Collaborator

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?

Copy link
Member Author

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! ✨

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a 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!

@cdransf cdransf merged commit 8aa61a9 into spectrum-two Nov 15, 2024
11 checks passed
@cdransf cdransf deleted the cdransf/s2-color-area-migration branch November 15, 2024 20:02
@github-actions github-actions bot mentioned this pull request Nov 12, 2024
castastrophe pushed a commit that referenced this pull request Dec 27, 2024
* 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
castastrophe pushed a commit that referenced this pull request Dec 29, 2024
* 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
castastrophe pushed a commit that referenced this pull request Dec 29, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants