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(colorloupe): migrate to s2 drop-shadow tokens #3301

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

rise-erpelding
Copy link
Collaborator

@rise-erpelding rise-erpelding commented Oct 22, 2024

Description

Migrates color loupe to Spectrum 2 tokens. According to the S2 token spec and S2 components changelog for color loupe, the only updated tokens concern the drop shadow. Color loupe now uses the new drop shadow tokens drop-shadow-elevated-x, drop-shadow-elevated-y, drop-shadow-elevated-blur, and drop-shadow-elevated-color rather than specific color loupe tokens.

CSS-707
S1 Guidelines: https://spectrum.adobe.com/page/color-loupe/

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

@marissahuysentruyt

  • Confirm in components/colorloupe/index.css that the color loupe component for S2 is using the tokens listed in the S2 token spec
  • Check out the branch locally or use the PR preview to view color loupe in Storybook. Inspect the component (search for the spectrum-ColorLoupe class if needed) and confirm that tokens, especially changed tokens, appear to match the S2 token specs

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

changeset-bot bot commented Oct 22, 2024

🦋 Changeset detected

Latest commit: 1749019

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

This PR includes changesets to release 5 packages
Name Type
@spectrum-css/colorloupe Major
@spectrum-css/colorhandle Major
@spectrum-css/colorwheel Major
@spectrum-css/colorarea Major
@spectrum-css/colorslider 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

@rise-erpelding rise-erpelding added size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. skip_vrt Add to a PR to skip running VRT (but still pass the action) S2 Spectrum 2 wip This is a work in progress, don't judge. labels Oct 22, 2024
Copy link
Contributor

github-actions bot commented Oct 22, 2024

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

Copy link
Contributor

github-actions bot commented Oct 22, 2024

File metrics

Summary

Total size: 4.29 MB*

🎉 No changes detected in any packages

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

@rise-erpelding rise-erpelding added the ask design Questions or topics for the design team label Oct 22, 2024
@rise-erpelding rise-erpelding marked this pull request as ready for review October 23, 2024 13:14
@rise-erpelding rise-erpelding self-assigned this Oct 23, 2024
@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-707-color-loupe-s2 branch from 8c4dd4d to e2b901a Compare October 23, 2024 13:17
@rise-erpelding rise-erpelding added ready-for-review and removed wip This is a work in progress, don't judge. labels Oct 23, 2024
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 looks great! Seems like it was a really simple migration- we probably won't get many of those! 😆

Maybe it's just a weird, rendering thing on my end, but does the docs color loupe not have the opacity checkerboard, while the default story does? The markup certainly looks identical to me (I ran it through diffchecker), but I don't know why the checkerboard isn't rendering on the docs instance.

Screenshot 2024-10-23 at 2 41 05 PM Screenshot 2024-10-23 at 2 40 57 PM

The docs color loupe just seems to behave a little differently to me, even when I change the stroke/stroke width on the svgs:

Screenshot 2024-10-23 at 3 11 40 PM Screenshot 2024-10-23 at 3 11 17 PM

I see both of these locally and in the PR preview. Any ideas for that? I'm not sure if that's something that would block the approval. 🤷‍♀️ I don't think I see either one of these issues on production so...maybe it's just an old bug. I'm approving anyways!

components/colorloupe/index.css Show resolved Hide resolved
components/colorloupe/index.css Show resolved Hide resolved
Comment on lines +23 to +24
--spectrum-colorloupe-outer-border-width: var(--spectrum-color-loupe-outer-border-width);
--spectrum-colorloupe-inner-border-width: var(--spectrum-color-loupe-inner-border-width);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we let design know that there's just a typo in the Border section of the tokens file? I think the second color-loupe-outer-border (which is supposed to be transparent black 200) is meant to say color-loupe-inner-border. Do they want to update and correct that?

@rise-erpelding
Copy link
Collaborator Author

rise-erpelding commented Oct 23, 2024

@marissahuysentruyt This is a known bug that I just noticed in Jira earlier today when I was looking for the design verification ticket! https://jira.corp.adobe.com/browse/CSS-775 It looks like it got closed out because it couldn't be reproduced, but I'm seeing it too in Chrome and Firefox with the url that points to the main branch, so it might need to be resurrected!

Maybe it's just a weird, rendering thing on my end, but does the docs color loupe not have the opacity checkerboard, while the default story does? The markup certainly looks identical to me (I ran it through diffchecker), but I don't know why the checkerboard isn't rendering on the docs instance.

Screenshot 2024-10-23 at 2 41 05 PM Screenshot 2024-10-23 at 2 40 57 PM
The docs color loupe just seems to behave a little differently to me, even when I change the stroke/stroke width on the svgs:

Screenshot 2024-10-23 at 3 11 40 PM Screenshot 2024-10-23 at 3 11 17 PM
I see both of these locally and in the PR preview. Any ideas for that? I'm not sure if that's something that would block the approval. 🤷‍♀️ I don't think I see either one of these issues on production so...maybe it's just an old bug. I'm approving anyways!

@rise-erpelding rise-erpelding force-pushed the rise-erpelding/css-707-color-loupe-s2 branch from e2b901a to 9200791 Compare October 30, 2024 18:08
@rise-erpelding rise-erpelding merged commit b91a202 into spectrum-two Nov 12, 2024
11 checks passed
@rise-erpelding rise-erpelding deleted the rise-erpelding/css-707-color-loupe-s2 branch November 12, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ask design Questions or topics for the design team ready-for-review S2 Spectrum 2 size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. 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.

3 participants