-
Notifications
You must be signed in to change notification settings - Fork 267
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
chore: link light mode #7216
chore: link light mode #7216
Conversation
@ekidneyrh I noticed in the design for #7149 that the color for the breadcrumb at the top left was defined, but this is a standalone Link component that is used everywhere we have links - dashboard, forms, deploy to Kube, etc. To support a hover effect on all background colors it currently uses 10% translucent white, so I've defined two colors: |
That sounds good to me! I'm not too picky on the names. |
It would be nice to use this opacity technique in other places for hover, and also for disabled components and text emphasis (some examples here: https://m2.material.io/design/color/text-legibility.html). That would help reduce the number of colors in the palettes |
Agreed. Ideally I'd like to use RGBA values but I can't find any way of cleanly specifying them in the registry or how we use them/Tailwind. I checked and the following does work since colors are just strings, but it feels a bit ugly. Am I overreacting, or can you think of a better way to do this?
|
It has to be checked for the difference cases, but it seems to me the opacity would always be the same for dark and light modes (and even the same if one changes the theme colors). I would simply put a This way, we could use, for example:
|
and for some components, it relies on accent-color |
I realized the background hover opacity (bg-opacity-10) wasn't working correctly, and the delay looking for a solution required a rebase to pick up Link move to UI package and changes to the color registry. The only path I've been able to find to get the background hover opacity is by defining this one color as a Tailwind color using the https://tailwindcss.com/docs/upgrade-guide#new-opacity-modifier-syntax @benoitf or @feloy would appreciate a quick comment to confirm this path is acceptable or if you can think of others. IMHO this path isn't too bad b/c we'll only have to define a couple colors this way. |
I would not add the color in tailwind as it won't be shared properly with other projects |
Any other ideas to set a hover bg opacity in Link? I haven't found any other path that allows you to use Tailwind's |
Because the
|
Adding light mode colors for Link component. bg-opacity-10 is deprecated and new /10 syntax is recommended, but neither work with colors defined as hex variables. In order to work I had to define the hover color as a Tailwind variable and use 'rgb(from var(' syntax. https://tailwindcss.com/docs/upgrade-guide#new-opacity-modifier-syntax Signed-off-by: Tim deBoer <git@tdeboer.ca>
Yeah, it does seem to be the simpler option atm, and would allow another theme to change the opacity as necessary. I have repushed the branch with this change. The closest value to 10% would actually be about hex 1A, but since black/white are three-hex values I tried both |
What does this PR do?
Adds light mode support to Link component, using color defined in design for #7149.
Screenshot / video of UI
No change on dark, looks like design on light - looks awful since I couldn't find a page with a Link that supports light mode, will look much better after #7149 has merged :) :
Screen.Recording.2024-05-14.at.1.42.07.PM.mov
What issues does this PR fix or reference?
Fixes #7215.
How to test this PR?
Manually add preference:
"preferences.appearance": "light"