-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
DataViews: Add Grid Layout #55353
DataViews: Add Grid Layout #55353
Conversation
<CardMedia> | ||
{ ( mediaField && | ||
mediaField.render( { item } ) ) || ( | ||
<Placeholder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's no "mediaField" or there's no featured image for the current post, we fallback to a placeholder.
<HStack justify="space-between" alignment="top"> | ||
<FlexBlock> | ||
<VStack> | ||
{ visibleFields.map( ( field ) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The visible fields are just rendered one after the other under the "media" and on the left of the "actions menu".
...fields.map( ( field ) => { | ||
const column = { ...field }; | ||
delete column.render; | ||
column.cell = ( props ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cell API is very weird and specific to tanstack, I've updated it to make it a bit clearer. I've called it "render" and it receives the "item" as a prop. This makes that function more generic and not specific to table cells.
I believe we might want to remove accessorFn
as well later, but didn't do it in the current PR.
Size Change: +438 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
{ visibleFields.map( ( field ) => ( | ||
<div key={ field.id }> | ||
{ field.render | ||
? field.render( { item } ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels it should be done in the main DataViews
component and only have one function to render. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are somethings to improve here. I think basically that we should remove accessorFn (mentioned some of this here #55353 (comment)) function from the fields config basically and yeah defining the fallback in the parent component should be doable.
Flaky tests detected in a14c48d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6544812500
|
This PR surfaced the need for a enableHiding feature specific to the view. It currently lives in the field definition by means of the tanstack API
I understand that different view types may want to have different hidable fields, because the essential information they display is different. For example, the essential information for a list/table is arguably the page's We could also consider having all fields as potentially hidable. My point is that if we allow one field not to be hidable because it's essential info in a certain view type (list/table), this is view-related config and not field-related config. |
@oandregal Good point 👍 It does mean "enableHiding" is probably not a "field" property but something in the "view" itself. |
Related to the above: the hiddenFields of the view also need to account for different view types. For example, it doesn't make sense that the |
Took this for a quick spin and have a few notes:
grid.view.movI'll leave out feedback about the design as I imagine that'll come in time :D |
Yes, that's because we're using the smaller sizes. The difficulty here is that we want the smaller sizes for the featured image when in the "table" view but want a bigger size when in the grid view. So I was wondering whether these should be two fields, a single field that adapt to the view type (right now, fields are layout independent) or something else. cc @oandregal @ntsekouras
I believe this might be something @ntsekouras is fixing separately
The fields render in the same way across layouts, so if it's linked in the table, it will be linked in the grid (at least for the initial version). About the sorting, for me I'm able to sort in both layouts using the menu top right. |
Yes, will be fixed here: #55387.
If we go for now with a required media item, I think it's okay to have an object and also pass the size(keep the small one as default). In general I'd avoid to introduce a |
@ntsekouras Haha, I actually went a head and added the "view" prop to the render functions right before you comment. Let me know if you disagree strongly :) |
We can iron out things as we go 😄 . You just need to rebase now that the fix of the failing tests landed. |
dcb38be
to
a14c48d
Compare
Ok, this is not perfect and there are still questions about:
Bu I believe we can land this as a basis and continue in follow-ups. WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge and iterate?
Related #55083
What?
This PR implements the initial render for the "grid view" in the DataViews component. Details in inline comments.
Testing Instructions
1- Open the page list pages in the site editor
2- Switch the layout type to "grid"
3- Try things like "field visibility", "sort", "pagination", "search" all of these should work as expected in both views.
Very early screenshot
This is a bit early obviously, but it's worth discussing the several inline comments I've added to the PR. It's a future defining PR in some aspects.