-
Notifications
You must be signed in to change notification settings - Fork 90
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
Show toolbox & distrobox containers different in containers listing #1409
Conversation
51b6ca2
to
35d42e9
Compare
Yeah, blue is probably too in-your-face. Grey works, but might be too understated. I wonder if any other color could work. (Green, orange, red are all probably out, as they often mean "good", "problem", and "error".) Perhaps a light purple or light cyan? Can you also add distrobox support? It should be the same, but say "distrobox" instead of "toolbox". Thanks! |
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.
LGTM, but needs a design review from @garrett
Seems it uses a different label? https://github.com/89luca89/distrobox/blob/3099d53a71ea5511eec23e83c33e25c032729e4f/distrobox-create#L579 |
A test would be great, just create a container with a label which matches toolbox's behaviour |
e0e65ca
to
8b0ff42
Compare
8b0ff42
to
15eaeec
Compare
Previously, I was thinking that we'd just use one color, because how many people use both toolbox and distrobox? I mean, I do, and perhaps others on the team as well... but we're probably not that ordinary. 😉 But this works for me. My one concern is the contrast ratio for the colors, primarily the gold color. If I plug the colors into https://accessible-colors.com/ — I get this: Which means we either need to make the background or foreground darker to have the correct color contrast ratio, so people can more easily read it. The purple also has the same problem, but not as much: We'll want to stick with the PF color palette (extended), so moving a shade down for the background colors of each would work... Or moving the shade up and flipping the text to dark instead of light. (At the size and weight do matter, but not at this size or smaller, which this is. For the purple to pass without color changes, it'd have to be 18px and bold... which we definitely don't want to do here.) https://www.patternfly.org/design-foundations/colors/#color-palette |
cc2a7c0
to
4c0c569
Compare
Fixed the contrast issues @garrett, new look: In dark mode it looks good: PS: Using https://accessible-colors.com/ the badge doesn't seem to have a good contrast. |
umm not sure why the tests failed... |
Thanks @garrett. So which one should we go for? Or use both? second one for light theme and first one for dark theme? |
I like the first set overall (dark on light), but if we have to do custom CSS anyway for assigning the colors, then we could do the light/dark, with the first set as light and second set as dark. |
4c0c569
to
10435bc
Compare
Thanks @garrett, it indeed looks cool now. Light: Dark: |
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! Thanks for working on this.
I've added a note about an alternate way to write the SCSS (which is probably how I would've done it), but it's totally fine as-is.
10435bc
to
1147609
Compare
Makes sense. It did cross my mind once to nest it, but I brushed it off for some reason :P... fixed it! thanks again! |
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.
Thanks
fixes #865
Show toolbox & distrobox containers different in containers listing