Skip to content
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: add new colors to the color registry #9420

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SoniaSandler
Copy link
Contributor

@SoniaSandler SoniaSandler commented Oct 16, 2024

Signed-off-by: Sonia Sandler ssandler@redhat.com

What does this PR do?

Adds new categories and colors to the color registry needed for #9080

The progress bar requires 3 main colors, and to make it easier to distinguish these colors based on use context, I have added:

  • background color for the incomplete part of the progress bar - gray
  • background color for the complete part - purple
  • text (if the progress bar has any related text) - purple

The extensions badges ()e.g. built in extension) in the extension page have hardcoded colors that don't exists in the color registry, and to make it easier if we would like to change them in the future, their respective background and text colors have been added to the registry:

  • bg and text for builtin extensions badge
  • bg and text for DD extension badge

Each of the providers in PD have a certain colors assigned to them, so I have added them to the registry based on ProviderInfoCircle.svelte:

  • Podman - purple
  • Docker - sky[400]
  • Kubernetes - sky[600]
  • unknown - gray

The services also have assigned colors in PD, so they also have been added to the registry based on ServiceColumnType.svelte:

  • ClusterIP - sky[500]
  • LoadBalancer - purple[500]
  • NodePort - fuschia[600]
  • unknown - gray

I have also added title-hover-bg so that the circle will be visible when hovering the buttons in the titlebar after changing it to use the color registry, and another background color to buttons based on the hardcoded colors was needed for the ProviderInstalled in the Dashboard page

Screenshot / video of UI

What issues does this PR fix or reference?

#9080

How to test this PR?

  • Tests are covering the bug fix or the new feature

Signed-off-by: Sonia Sandler <ssandler@redhat.com>
@deboer-tim
Copy link
Contributor

This PR add several new colors, but it's disconnected from why we'd need to add or change them, or how it is going to be used. IMHO that would make it hard to review when it comes out of draft. I think you either need to break it up along functional lines (e.g. progress bar color + changes to progress bar), or explain/reference the design for each bit to show we're missing a color that we're going to need.

The first will be painful because of multiple PRs on color-registry.ts which could take longer and potentially have some merge conflicts; the second more work up front to explain why we need each change. :-/

@SoniaSandler
Copy link
Contributor Author

@deboer-tim I updated the PR description. lmk if that works

@SoniaSandler SoniaSandler marked this pull request as ready for review October 18, 2024 12:26
@SoniaSandler SoniaSandler requested review from benoitf and a team as code owners October 18, 2024 12:26
@SoniaSandler SoniaSandler requested review from dgolovin, jeffmaury and feloy and removed request for a team October 18, 2024 12:26
@deboer-tim
Copy link
Contributor

@deboer-tim I updated the PR description. lmk if that works

Thanks, that is much better but tbh I still don't know how to review as one chunk. e.g.:

  • progress bar - No concerns, but should get a quick nod from @ekidneyrh, and maybe a future issue to add to the design [1]. Being the same color in dark vs light is probably an indication that the light colors haven't been picked yet.
  • extension badges - Ditto.
  • titlebar - Ditto, we just missed a color before?
  • button - I don't remember a tertiary color in the design or understand where it is missing.
  • providers - Hmmm, do we really want people to customize podman to yellow or docker to red? This might take some discussion; maybe we should just be hardcoding the correct RGB value or allowing provider to supply?
  • services - Seems very specific and wouldn't cover any changes, nor other pages that need similar badges/dots. Maybe we should merge this and the extension badges and just have a set of complimentary badge colors that can be used by any page?

[1] https://github.com/containers/podman-desktop/wiki/Design-System-Theming

So for me, there is still a lot grouped together here and the simple things won't get merged until we decide/agree on everything. I would still recommend either breaking this up into functional chunks (e.g. tertiary color + the code that uses it) and deal with the inevitable merge conflicts as each gets approved, or at least separate the more obvious/simple things where we can fix the light colors and get it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants