-
Notifications
You must be signed in to change notification settings - Fork 201
feat(modal): diy migration #2164
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
🚀 Deployed on https://pr-2164--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.
Just a few comments. Don't forget to add --mod
variables to all of these!
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 a few things to fix up here, @TarunAdobe. We have some tokens for animations that we defined here: https://github.com/adobe/spectrum-css/blob/a0ff7502807b1b6e464a330574fa909024a18bf6/components/tokens/custom-spectrum/custom-vars.css#L20C3-L37 - that's where I pulled my suggestions from.
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 looks good to me.
My only suggestion is to rename the following properties to logical properties.
b61194d
to
f68fc20
Compare
chore(modal): fixed indentation index.css chore(modal): added mod variables chore(modal): updated package version chore(modal): updated css to use custom tokens for animation chore: updated css properties
f68fc20
to
54377c3
Compare
Description
Migrating modal component to diy core tokens.
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.
Regression 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. ✨