-
Notifications
You must be signed in to change notification settings - Fork 356
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
refactor: update UI Kit Icon component [WEB-1699] #8122
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
backgroundColor="white" // only gets applied for scheduled and queued states | ||
opacity={0.25} // only gets applied for scheduled and queued states |
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.
I couldn't figure out how to get the Queued animation to look right without these styles, but if anyone else has an idea please let me know.
52dd53e
to
fdf7b98
Compare
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.
looks great, a few comments but nothing blocking merge.
03fa8dc
to
1cdbe89
Compare
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.
Icon changes look good.
Description
This PR:
Note: The ticket calls for our Logo component to be merged into Icon. I haven't done that yet because Logo has dependencies on our determinedInfo store and handles light vs dark theming and sizing differently than the rest of the icons.
Test Plan
Commentary (optional)
Checklist
docs/release-notes/
.See Release Note for details.
Ticket