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: unify duplicate code in FormPage #7538

Merged
merged 5 commits into from
Jun 20, 2024
Merged

Conversation

lstocchi
Copy link
Contributor

What does this PR do?

This PR is made in preparation to fix #7214
By looking at the code of each component using the FormPage i noticed that we were using almost everywhere

<div slot="content" class="px-5 pb-5 min-w-full h-fit">
    <div class="bg-charcoal-900 px-6 py-4 space-y-2 lg:px-8 sm:pb-6 xl:pb-8 rounded-lg">
    .....

or something very close.

This PR updates the FormPage to accept a value to customize the layout. When the value engine is used those div/classes are automatically set so to reduce the duplicate code in all pages using it.

You will see a lot of code changed but it is mostly a change in the indentation. i just deleted the useless wrapping classes/div

Screenshot / video of UI

N/A - should be the same as before

What issues does this PR fix or reference?

it is part of #7214

How to test this PR?

N/A - it should be like before

  • Tests are covering the bug fix or the new feature

@lstocchi lstocchi requested review from benoitf and a team as code owners June 10, 2024 12:20
@lstocchi lstocchi requested review from cdrage, axel7083 and deboer-tim and removed request for a team June 10, 2024 12:20
Comment on lines 34 to 36
{#if showEmptyScreen}
<NoContainerEngineEmptyScreen />
{:else}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not see why we need to have a 'layout' enum property inside this component

it's not this component that should do the change and display a nocontainerengine but the callee/parent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put this here bc the FormPage is already a wrapper for the FormPage from the ui package, so this is really tight to podman desktop and its needs. There are multiple components using the same emptyscreen/content pattern so i thought that the formPage could have different layouts and handle it.
Do you prefer every single component to handle its own css/empty screen and leave the formPage as it was?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is some small code savings but feels odd to me as well - saying 'engine' changes the layout, and we have a fixed empty screen but only show it based on a reason the page has to provide. Maybe it'd work better as an EngineFormPage that extends FormPage and automatically shows the empty screen when needed? (or whenever the slot isn't provided?) It currently feels more like 'hard-coded requirements for specific children' than 'optional feature' like tabs or breadcrumb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an EngineFormPage component

@lstocchi lstocchi requested a review from benoitf June 11, 2024 11:27
Copy link
Contributor

@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.

I've been looking at the component hierarchy and some of the history here, and child pages set their own background and rounded corners on the form because originally this was the same parent as the dashboard or troubleshooting where we couldn't draw the tiles because we didn't know how many there would be on the page.

IMHO it's time to clean that up - FormPage should exist to provide a single form for the user to fill out, sub-files should just be adding content to the form.

What I think that means:

  • Troubleshooting & Help should be changed or moved off of FormPage to avoid breakage in the next steps. Likely Troubleshooting to DetailsPage and Help to NavPage?
  • bg-charcoal-900 & rounded-lg - should be moved 'up' to FormPage.
  • All 5 p* classes - should also be moved to FormPage, but simplified to just 'p-5' (or the different px/py) b/c we don't need this to be responsive and were only doing it for 1 page out of ~10.
  • If we need empty screen to not be 'in' the form, FormPage should provide an alternate slot.
  • It feels like the form could be setting the space-y instead of having this different in each form.

So how does this PR fit? I'm ok if this is a stepping stone to get to the above, as it cleans up most of the work in this set of child pages. The only things I don't love:

  • Inconsistent space-y - could be opened as future issue.
  • it feels like the showEmptyScreen should be automatic: i.e. this property doesn't exist and all pages get the empty screen if there are no providers. Again, I'm ok if this was a follow up.
  • The simplified padding. Could you just simplify this before merging?

lstocchi added 3 commits June 20, 2024 09:44
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
lstocchi added 2 commits June 20, 2024 09:58
Signed-off-by: lstocchi <lstocchi@redhat.com>
Signed-off-by: lstocchi <lstocchi@redhat.com>
@lstocchi
Copy link
Contributor Author

@deboer-tim ok for me. Not sure about the space-y as it changes the layout but we can check it in separate PR.

Rebased, fixed an issue on the runimage page and simplified the padding.

@lstocchi lstocchi merged commit 1dbd704 into podman-desktop:main Jun 20, 2024
7 checks passed
@lstocchi lstocchi deleted the i7214 branch June 20, 2024 09:16
@podman-desktop-bot podman-desktop-bot added this to the 1.11.0 milestone Jun 20, 2024
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.

Light mode for individual forms
5 participants