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

feat: enable light mode for buildImageFromContainerfile page #7773

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

Conversation

lstocchi
Copy link
Contributor

@lstocchi lstocchi commented Jun 21, 2024

What does this PR do?

This PR enables light mode in the build image from container file page.
It is built over #7768

Screenshot / video of UI

buildimagefromcontainerfile

What issues does this PR fix or reference?

it is part of #7214

How to test this PR?

go to images -> click on build button (top right)

  • Tests are covering the bug fix or the new feature

@lstocchi lstocchi marked this pull request as ready for review June 21, 2024 15:14
@lstocchi lstocchi requested review from benoitf and a team as code owners June 21, 2024 15:14
@lstocchi lstocchi requested review from jeffmaury, axel7083 and deboer-tim and removed request for a team June 21, 2024 15:14
Copy link
Collaborator

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM and worked well in testing. My only concern is if we should really have --pd-formpage-card*, i.e. whether these should be generic cards (variable names without formpage) or there are existing color variables we should use. @ekidneyrh any preference?

@jeffmaury jeffmaury changed the title feat: enable light mode fro buildImageFromContainerfile page feat: enable light mode for buildImageFromContainerfile page Jun 24, 2024
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Same remarks about reusing variables

@lstocchi
Copy link
Contributor Author

lstocchi commented Jun 25, 2024

Not sure how to change the variables name. By looking at other cases i see that the cards used for carousel have content-card-carousel-*, then we have generic content card content-card-*, then card related to invert-content- have prefix invert-content-card-*. So based on their scope we have different prefixes. As these are related to formpage i thought that using formpage prefix made sense.
So which are your suggestions?
By looking at the code, they are actually buttons <button></button> styled like cards. Is content-button-card-* good enough ?? Anything else?

@deboer-tim @jeffmaury

@deboer-tim
Copy link
Collaborator

Not sure how to change the variables name. By looking at other cases i see that the cards used for carousel have content-card-carousel-*, then we have generic content card content-card-*, then card related to invert-content- have prefix invert-content-card-*. So based on their scope we have different prefixes. As these are related to formpage i thought that using formpage prefix made sense. So which are your suggestions? By looking at the code, they are actually buttons <button></button> styled like cards. Is content-button-card-* good enough ?? Anything else?

Our design has most items that always look the same no matter where they are (Buttons, Inputs, Checkboxes, etc), and then some things like content cards that are different depending on context like Dashboard vs Settings. If the design/colors for the platform cards don't match tiles like on the dashboard or elsewhere then I'm fine if they are new vars just for forms, but card-input-bg is definitely 'wrong' because inputs shouldn't change based on where they are. Can you pick up some of the input-field colors instead or switch to use Input instead of defining a new color?

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.

None yet

3 participants