-
Notifications
You must be signed in to change notification settings - Fork 201
refactor(assetlist)!: migrate to spectrum tokens #2211
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
Conversation
59471dc
to
3288213
Compare
🚀 Deployed on https://pr-2211--spectrum-css.netlify.app |
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 with this, including the accessibility fixes! I've confirmed the validation steps and everything looks right, and improved.
I don't think --spectrum-assetlist-checkbox-margin
is used, so that can probably be removed?
My only suggestion would be to pass an id
into the Checkbox story in Storybook, as Chrome console shows a recommendation about it not having an id
or name
(while also saying it's not strictly necessary)---the docs examples do already have an ID on them, so that would help align the markup.
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.
Definitely an improvement on dark
and darkest
which look broken in production...
Thanks for taking care of this!
bd29e4a
to
99767dd
Compare
Description
Migrates AssetList to spectrum tokens
is-navigated
AssetList-Item to docs siteHow 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.
Test Outline:
@jawinn
is-navigated
stylingRegression testing
Validate:
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. ✨