-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Navigation] Fix icon filter colors #4664
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
| --p-filter-icon: brightness(0) saturate(100%) invert(36%) sepia(13%) | ||
| saturate(137%) hue-rotate(169deg) brightness(95%) contrast(87%); | ||
|
|
||
| @include recolor-icon(var(--p-icon), transparent, var(--p-icon-filter)); | ||
| --p-filter-icon-action-primary: brightness(0) saturate(100%) invert(20%) | ||
| sepia(59%) saturate(5557%) hue-rotate(162deg) brightness(95%) contrast(101%); | ||
| --p-filter-icon-on-interactive: brightness(0) saturate(100%) invert(100%); |
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.
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. Thanks @kyledurand.
|
|
||
| .SecondaryAction { | ||
| @include recolor-icon(var(--p-icon)); | ||
| @include recolor-icon($fill-color: var(--p-icon)); |
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.
Switching to named vars throughout this PR for clarity. Should make things slightly easier to figure out what's going on when we remove recolor-icon()
| items={[ | ||
| { | ||
| url: '/path/to/place', | ||
| url: '/', |
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.
We always have one active items in the nav within the admin. This change reflects that int he basic example
| items={[ | ||
| { | ||
| url: '/', | ||
| url: '/path/to/place', |
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.
We had two active items in this example which should never happen. Undoing this one since the example is for selected secondary, which is below
size-limit report
|
aaronccasanova
left a comment
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.
Updates look great! Note: I was mainly focused on validating the syntax and consistency of the updates since I am unfamiliar with this component and the recolor-icon mixin.
WHY are these changes introduced?
Fixes #4659
WHAT is this pull request doing?
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes