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

New Admin UI - Grid / Col Components #4252

Open
wants to merge 9 commits into
base: feat/new-admin-ui
Choose a base branch
from
Open

Conversation

adrians5j
Copy link
Member

@adrians5j adrians5j commented Sep 6, 2024

This PR introduces the Grid / Cell components.

Other Changes

1. Elevation Fix

Found an issue with the Elevation component (not able to receive custom CSS class). Resolved now.

2. Switch Fix

Noticed a TS issue related to the Switch component. Applied this fix.

3. svg HTML Element Displayed As Block

This is not a change, but a discovery. I've discovered Tailwind's preflight makes all svg HTML tags display as blocks by default. Which previously was not the case, and now, for example, in the dashboard, you can noticed icons in the bottom-right being aligned to the left, and not to center. For now did not do anything, just wrote it down in GH project as a reminder.

4.withStaticProps Utility

Added this utility to easily add static props to React components. This helps in situations where you have a for example Grid component, and you also want to have Grid.Column. The same was used with the Avatar component.

@adrians5j adrians5j changed the base branch from next to feat/new-admin-ui September 6, 2024 11:27
Copy link

Cypress E2E tests have been initiated (for more information, click here). ✨

@adrians5j
Copy link
Member Author

adrians5j commented Sep 10, 2024

Couple of comments...

  1. Not sure if Switch TS issue was addressed @leopuleo ? I have a feeling I saw it being addressed by you, but don't know anything anymore. 😆

  2. Do we want to be able to create responsive grids? Like for example, show 12-columns-grid in wide screen, and 1-column-grid on a small screen?

@adrians5j adrians5j marked this pull request as ready for review September 10, 2024 15:05
@adrians5j
Copy link
Member Author

adrians5j commented Sep 10, 2024

Re number 2... I would've already done it... but I'm actually having some trouble with the API design and the underlying implementation. I've seen that some libs have the following approach when it comes to responsive grids:

// 12 columns for all screen sizes.
<Grid columns={12}>...</Grid> 

// Use 12 columns for all sizes, except small screens, where we use 1.
<Grid columns={{ initial: 12, sm: 1 }}>...</Grid> 

But now.... the only thing that then becomes a bit complicated here is class generation with cva. Basically the lib only allows us to assign POJO when defining a variant, like:

   columns: {
            1: "grid-cols-1",
            2: "grid-cols-2",
            3: "grid-cols-3",
            4: "grid-cols-4",
            5: "grid-cols-5",
            6: "grid-cols-6",
            7: "grid-cols-7",
            8: "grid-cols-8",
            9: "grid-cols-9",
            10: "grid-cols-10",
            11: "grid-cols-11",
            12: "grid-cols-12"
        },

Looks to me we'd need to handle the above {{ initial: 12, sm: 1 }} proposal without cva. But even by doing that, still not sure how to deal with all of the TW classnames. I mean, they would still need to be listed somewhere as strings, so that they are all included in the CSS bundle ultimately.

Maybe we need a different API here? 🤔

Or maybe we don't even need the Grid component at all, but we use it just for old Admin UI? And we simply use TW classes directly? Not sure all in all.

@adrians5j adrians5j changed the title New Admin UI - Grid / Cell Components New Admin UI - Grid / Col Components Sep 25, 2024
@adrians5j
Copy link
Member Author

An update on grid vs. flex (there are still other items to resolve in this PR btw, this is not the last one):

So I tried using flex, and here's my observation.

Before I start, here are the results I've achieved:
image

Also, for reference, old sshot showing the same thing done with regular grid (current implementation in this PR):
image

So, what I've found is that the flex-version of the grid was not that great. The first version of the code produced the following result:

image

As we can see, the result is nowhere near the desired output. This is not only because the existing @webiny/ui package's Grid/Col components are also using grid instead of flexbox, but also, because this dashboard code uses the Grid component and inside of it, just contains multiple columns, that are spanning across 24columns in total. And then, they are broken into 2 rows, that's why it looks like a grid.

Check this code here to get the full picture, I may not have explained it well:

Ultimately, a bit of code shuffling was needed to get the desired result. Basically, I needed to ensure that each row consists of 12 columns. If the row has more than 12 columns, then it should be broken into multiple rows.

Once I did that, I receive the following:
image

As we can see, that's better. Still, there will be a lot of views that will need to get refactored. For example, I just opened 'new content model' dialog:

image

With the grid-approach (code in this PR):
image

One other thing I noticed is how gap affects the layout. Here's the same layout with a gap set to 12, with the grid approach:

image

And here's the same with flex approach:
image

As we can see, with flex approach, we receive a horizontal scrollbar, because it looks like the gap making the columns wider than the container. This is not the case with the grid approach. So, this is something to keep in mind.

In both cases, I've tried different things, like shrink and flex-wrap, but could not get it to work.

In any case, didn't want to move in any approach before we both agree. If you ask me, I'd just continue with the initial grid approach, and move on. We can always address this later, if needed.

@adrians5j
Copy link
Member Author

BTW some other items I have on the list are...

  1. Remove columns from the main Grid component. With this initial version, we keep it at 12 always.
  2. Check offseting.
  3. Ignore responsive, we just do one mode.

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

Successfully merging this pull request may close these issues.

1 participant