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.

Sign up for GitHub

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

Jump to bottom

[poc] Add layout components #6239

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

[poc] Add layout components #6239

wants to merge 9 commits into from

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Mar 2, 2024 •
edited

What does this PR do?

  • Add a HorizontalCard component, and use it for PreferencesCliTool and PreferencesResourcesRendering components
  • Add a Card component, and use it in different places
  • Add a CardTitle component, and use it in Card and HorizontalCard
  • HorizontalCard, Card and CardTitle support dark/light mode (except icon)

What issues does this PR fix or reference?

How to test this PR?

  • Tests are covering the bug fix or the new feature

@feloy feloy requested review from benoitf and a team as code owners March 2, 2024 11:25
@feloy feloy requested review from dgolovin and axel7083 and removed request for a team March 2, 2024 11:25
@feloy feloy marked this pull request as draft March 2, 2024 11:26
@feloy feloy changed the title Add a HorizontalCard component Add a layout components Mar 2, 2024
@feloy feloy changed the title Add a layout components Add layout components Mar 2, 2024
benoitf
benoitf reviewed Mar 3, 2024
View reviewed changes
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

introducing component is looking a good enhancement but it should be done by UX team before

It should define the colors, how it looks, etc.

packages/main/src/plugin/color-registry.ts
Comment on lines +344 to +353
this.registerColor(`${invCt}card-bg-highlighted`, {
dark: colorPalette.charcoal[700],
light: colorPalette.gray[200],
});

this.registerColor(`${invCt}card-divide`, {
dark: colorPalette.gray[900],
light: colorPalette.charcoal[50],
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

Look nice to introduce new components

but you can't introduce new components/colors "like that"

the colors are defined in the Podman Desktop design system

And there is not yet these colors or these components

if you need to extract values, it should be defined before the implementation

packages/renderer/src/lib/dashboard/DashboardPage.svelte
Comment on lines +100 to +111
</div>
<div class="px-2 mt-5 h-full">
<Card title="Learning center">
<svelte:fragment slot="content">
<LearningCenter />
</svelte:fragment>
</Card>
<Card title="Featured extensions">
<svelte:fragment slot="content">
<FeaturedExtensions />
</svelte:fragment>
</Card>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be really cautious there

For example Learning center does not match the expected design
see #6221

packages/renderer/src/lib/preferences/PreferencesExtensionList.svelte
Comment on lines +81 to +89
<Card title="Featured Extensions">
<svelte:fragment slot="content">
<FeaturedExtensions />
</svelte:fragment>
</Card>

<Card title="Install a new extension from OCI Image" label="OCI image installation box">
<svelte:fragment slot="content">
<div class="flex flex-col w-full">
Copy link
Collaborator

Choose a reason for hiding this comment

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

this screen is being changed as part of #2170
so I'm not sure there should be changes for now in that page

@benoitf
Copy link
Collaborator

benoitf commented Mar 3, 2024

What is the rationale of components and the slots

for example are we not missing CardHeader, CardBody components ?

@axel7083
Copy link
Contributor

axel7083 commented Mar 4, 2024

We might want take some inspiration from existing library and implementation

Some remarks

  • The content of the card (horitontal, vertical), should not be choosed by a card component, it should be the responsability of the component definiting the content, so if I want to make it a grid, a flex, of anything else I am not bounded by the component choices.
  • Use its own folder in the ui library for Card (see example in github repo linked above)

@feloy feloy changed the title Add layout components [poc] Add layout components Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benoitf benoitf benoitf left review comments

@dgolovin dgolovin Awaiting requested review from dgolovin dgolovin is a code owner automatically assigned from containers/podman-desktop-reviewers

@axel7083 axel7083 Awaiting requested review from axel7083 axel7083 is a code owner automatically assigned from containers/podman-desktop-reviewers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@feloy @benoitf @axel7083