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

Use FormPage for Import Model page #1230

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Jun 18, 2024

What does this PR do?

Updtae the "Import Model" page to use the dedicated FormPage component instead of NavPage

Also change a few colors to support light mode (except colors in the form itself)

Screenshot / video of UI

What issues does this PR fix or reference?

Fixes #1229

How to test this PR?

Navigate to Catalog > Import

@feloy feloy requested review from benoitf and a team as code owners June 18, 2024 13:17
@feloy feloy requested a review from axel7083 June 18, 2024 13:17
Copy link
Contributor

@axel7083 axel7083 left a comment

Choose a reason for hiding this comment

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

One small issue

Before

dark light
image image

After

dark light
image image

We have a clear improvement on the light mode (nice job), but we loose the separation between the secondary navigation and the content on both theme

@@ -34,7 +34,7 @@ onMount(() => {
</script>

<Route path="/*" isAppMounted="{isMounted}" let:meta>
<main class="flex flex-col w-screen h-screen overflow-hidden bg-charcoal-700">
<main class="flex flex-col w-screen h-screen overflow-hidden bg-[var(--pd-content-bg)]">
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if we should not use var(--pd-invert-content-bg) as in the preferences, it is what is used however this would make the card of the form hidden :/

@axel7083 axel7083 requested a review from ekidneyrh June 18, 2024 13:37
@feloy
Copy link
Contributor Author

feloy commented Jun 18, 2024

One small issue

[...]
We have a clear improvement on the light mode (nice job), but we loose the separation between the secondary navigation and the content on both theme

Do you mean the vertical shadow between the 2 zones? If so, this issue tracks the problem: containers/podman-desktop#7549

Signed-off-by: Philippe Martin <phmartin@redhat.com>
@feloy feloy force-pushed the fix-1205/form-page-model-import branch from 053fb3d to 40f8b2c Compare June 18, 2024 13:38
@axel7083
Copy link
Contributor

Do you mean the vertical shadow between the 2 zones? If so, this issue tracks the problem:

Sorry if I was not clear enough, the fact that the main and secondary navigation are both the same color:

image

@feloy
Copy link
Contributor Author

feloy commented Jun 18, 2024

Do you mean the vertical shadow between the 2 zones? If so, this issue tracks the problem:

Sorry if I was not clear enough, the fact that the main and secondary navigation are both the same color:

image

I double-checked, and for me the 2 colors were already before really the same (#343434), but the shadow in the middle makes one think they are different

@feloy feloy merged commit b71cfb8 into containers:main Jun 19, 2024
4 checks passed
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.

use FormPage for Import Model page
2 participants