Skip to content

feat(dropindicator): migrate to spectrum tokens #2198

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 8, 2023
Merged

feat(dropindicator): migrate to spectrum tokens #2198

merged 1 commit into from
Nov 8, 2023

Conversation

jnjosh
Copy link
Collaborator

@jnjosh jnjosh commented Oct 2, 2023 •
edited
Loading

Description

Migrate Drop Indicator to Spectrum Tokens

How and where has this been tested?

  1. Open the storybook for the drop indicator component:
    • Validate that horizontal and vertical match on colors and circle sizes @jenndiaz
    • Introduce a mod and verify it mods approppriately? @jenndiaz

Regression testing

Validate:

  1. A legacy documentation page (such as accordion), including:
  • The page renders correctly
  • The page is accessible
  • The page is responsive
  1. A migrated documentation page (such as action group), including:
  • The page renders correctly
  • The page is accessible
  • The page is responsive

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.
  • ✨ This pull request is ready to merge. ✨

@jnjosh jnjosh self-assigned this Oct 2, 2023
@jnjosh jnjosh added the run_vrt For use on PRs looking to kick off VRT label Oct 2, 2023
jnjosh
jnjosh commented Oct 2, 2023
View reviewed changes
jnjosh
jnjosh commented Oct 2, 2023
View reviewed changes
@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023 •
edited
Loading

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

@github-actions github-actions bot temporarily deployed to pull request October 2, 2023 15:02 Inactive
@github-actions github-actions bot removed the run_vrt For use on PRs looking to kick off VRT label Oct 2, 2023
jnjosh
jnjosh commented Oct 2, 2023
View reviewed changes
@pfulton pfulton requested review from jawinn and jenndiaz October 3, 2023 13:22
jenndiaz
jenndiaz suggested changes Oct 3, 2023
View reviewed changes
@jnjosh jnjosh force-pushed the jnjosh/CSS-510-migrate-dropindicator branch from 11926c5 to 93a7ba6 Compare October 13, 2023 17:38
@github-actions github-actions bot temporarily deployed to pull request October 13, 2023 17:43 Inactive
@jnjosh jnjosh force-pushed the jnjosh/CSS-510-migrate-dropindicator branch from 93a7ba6 to e506a23 Compare October 13, 2023 17:46
@jnjosh jnjosh requested a review from jenndiaz October 13, 2023 17:47
@github-actions github-actions bot temporarily deployed to pull request October 13, 2023 17:52 Inactive
jenndiaz
jenndiaz approved these changes Oct 16, 2023
View reviewed changes
Copy link
Contributor

@jenndiaz jenndiaz 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!

@jnjosh jnjosh force-pushed the jnjosh/CSS-510-migrate-dropindicator branch from e506a23 to 633271f Compare October 16, 2023 16:41
@github-actions github-actions bot temporarily deployed to pull request October 16, 2023 16:47 Inactive
pfulton
pfulton requested changes Oct 24, 2023
View reviewed changes
Copy link
Collaborator

@pfulton pfulton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I know that they didn't exist before, but I think we should address Windows High Contrast Mode/forced-colors in this PR.

Thankfully, the amount of colors here are limited, so there will probably only be a few that will need to be accounted for. We have details on the wiki about WHCM, and our other components should be able to serve as a guide for what the syntax looks like.

Could you take a shot at taking care of those WHCM styles and pushing that up?

For reference, this is what the component looks like with forced-colors: active:
Screenshot 2023-10-24 at 3 43 39 PM

@jnjosh jnjosh force-pushed the jnjosh/CSS-510-migrate-dropindicator branch from 633271f to 66e629e Compare October 25, 2023 17:07
@jnjosh
Copy link
Collaborator Author

jnjosh commented Oct 25, 2023

I know that they didn't exist before, but I think we should address Windows High Contrast Mode/forced-colors in this PR.

Thankfully, the amount of colors here are limited, so there will probably only be a few that will need to be accounted for. We have details on the wiki about WHCM, and our other components should be able to serve as a guide for what the syntax looks like.

Could you take a shot at taking care of those WHCM styles and pushing that up?

For reference, this is what the component looks like with forced-colors: active: Screenshot 2023-10-24 at 3 43 39 PM

@pfulton Thanks for the feedback! I've updated the PR to include a style for that mode as can be seen here.

Screenshot 2023-10-25 at 1 03 34 PM

@jnjosh jnjosh requested review from pfulton and jenndiaz October 25, 2023 17:09
@github-actions github-actions bot temporarily deployed to pull request October 25, 2023 17:13 Inactive
pfulton
pfulton approved these changes Oct 25, 2023
View reviewed changes
Copy link
Collaborator

@pfulton pfulton 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. Thanks for making the WHCM fix!

jenndiaz
jenndiaz suggested changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@jenndiaz jenndiaz left a comment

Choose a reason for hiding this comment

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

I just had one comment.

@jnjosh jnjosh force-pushed the jnjosh/CSS-510-migrate-dropindicator branch from 66e629e to 823ffa3 Compare October 26, 2023 02:56
@jnjosh jnjosh requested a review from jenndiaz October 26, 2023 02:56
@github-actions github-actions bot temporarily deployed to pull request October 26, 2023 03:01 Inactive
jenndiaz
jenndiaz approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@jenndiaz jenndiaz 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!

@mdt2 mdt2 mentioned this pull request Nov 2, 2023
Merged
15 tasks
@jnjosh jnjosh force-pushed the jnjosh/CSS-510-migrate-dropindicator branch from 823ffa3 to 42e491b Compare November 6, 2023 20:51
@github-actions github-actions bot temporarily deployed to pull request November 6, 2023 20:54 Inactive
@pfulton pfulton force-pushed the jnjosh/CSS-510-migrate-dropindicator branch from 42e491b to 163ec2c Compare November 8, 2023 14:26
@pfulton pfulton added the run_vrt For use on PRs looking to kick off VRT label Nov 8, 2023
@github-actions github-actions bot temporarily deployed to pull request November 8, 2023 14:31 Inactive
@github-actions github-actions bot removed the run_vrt For use on PRs looking to kick off VRT label Nov 8, 2023
@pfulton pfulton merged commit da24515 into main Nov 8, 2023
@pfulton pfulton deleted the jnjosh/CSS-510-migrate-dropindicator branch November 8, 2023 14:42
@github-actions github-actions bot mentioned this pull request Apr 19, 2024
Merged
@github-actions github-actions bot mentioned this pull request May 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pfulton pfulton pfulton approved these changes

@jenndiaz jenndiaz jenndiaz approved these changes

@jawinn jawinn Awaiting requested review from jawinn

Assignees

@jnjosh jnjosh

Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jnjosh @pfulton @jawinn @jenndiaz