Dataviews: Grid layout refinements #56441
Conversation
ntsekouras
added
Needs Design Feedback
ntsekouras
requested review from
youknowriad,
oandregal,
jameskoster and
SaxonF
|
Size Change: +365 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
A few visual polish and refinements still to make (I'll push changes when I get a moment) but this is looking better already! :) Ideally the title has a different treatment compared to other fields (e.g. larger font size, hidden label, etc), will that be feasible? Would it also be possible for the entire grid item to act as a link (same destination as the title)? Keeping in mind that we also want each field to be inline-editable in the future. If we have to prioritise one over the other then making the whole grid item a link seems more important to me. |
I think that's better to discuss separately. An idea was tossed somewhere about having a primary field(like the title) and we could have some different treatment to that. It will be easy enough if we make an API like that. Regarding the whole item to be a link, I think we can try it in this PR but I'm not sure how the current field values will look like(ex links etc..). We'll see. Maybe in the future when fields are editable, we have an |
|
Personally, I prefer the trunk design but I guess I'm not the one to decide here:
Botderless feels lighter to me
This doesn't look great to me, I don't like how titles are on the left and values on the right.
Not too opinionated. Just my 2cents, whatever designers want here, I'm fine with it but wanted to share my personally impression. |
|
I'll leave design to speak/approve this one, though it can land on my opinion. I like how it introduces some visual order. Design-wise, I noticed there is the wide separation between the field title (left) and the field value (right), making them disconnected in the current 2-columns layout: 
|
|
I think the decision around whether to add borders or not is linked to whether or not the entire grid item can serve as a link. If it can, then the borders make more sense. If we're not going to explore that as a part of this PR (seems reasonable) let's remove the borders for now. I'm going to work on some other style adjustments now. |
|
I pushed a couple of tiny adjustments, but seeing this in practise makes me think the general card design needs a little more time in the oven. I'll work on that in Figma and can follow-up with another PR when ready. One thing we should probably address here is the "Featured Image" view option when grid layout is selected. Currently you can toggle it on/off but it does nothing. We could either disable it in the In terms of follow-ups, I think treating the title differently will be quite important as mentioned above. |
ntsekouras
force-pushed
the
refine/dataviews-grid
branch
from
8f5cca1 to
f8787e0
Compare
ntsekouras
force-pushed
the
refine/dataviews-grid
branch
from
7ea3a67 to
f9af337
Compare
|
@ntsekouras thanks for adding the Edit: Another quirk I noticed while working on some styling: If Grid is the default layout for a view then the previews do not load. But if you switch to list layout then back to grid layout they do. |
|
Pushed some styling adjustments, the Pages view is getting there (especially when #56501 is merged), though still some work to do. 
Templates still needs some fine tuning, here a couple of items I'd really appreciate help with:
|
ntsekouras
force-pushed
the
refine/dataviews-grid
branch
from
2cffe7c to
629909b
Compare
| <div className="dataviews-view-grid__media"> | ||
| { mediaField?.render( { item, view } ) || ( | ||
| <Placeholder | ||
| withIllustration |
There was a problem hiding this comment.
@jameskoster this used to render a Placeholder and now we just have the empty box-shadowed div. Is that the wanted design?
|
Thanks for pushing the updates @jameskoster! There are some issues with this approach that are visible in the templates list. With this implementation(css styling) we add the separator between every field, but some fields render nothing. This might not be easy to even handle in JS, as the
If we make it a specific field, it cannot render inside another field and will have to be shown separately. We could explore how to support extra fields that are hidden, but being able to add filters etc.. Alternatively we can hide the
Any changes made in |
ntsekouras
force-pushed
the
refine/dataviews-grid
branch
2 times, most recently
from
2d89fb6 to
ddfe92a
Compare
|
Thanks for all the clarification @ntsekouras. If I'm understanding correctly it would make sense to keep the field + title visible in this PR, then circle back later to consider expanding the API to handle more nuanced design (e.g. hiding titles where not necessary). Assuming that's correct I'll revert some of the changes I made. Totally fine to handle the 'customized' field separately too. |
You mean the field label and remove the Tooltip? If yes, maybe let's try first if we can do differently(not in css file). |
|
I don't think we can hide the label without also including the tooltip because some fields cannot be interpreted without a label e.g. publish date. So I'll reinstate the label for now. Later perhaps we can consider a way to show/hide labels per field in grid view. Edit: A 'hide when empty' option might be useful too. The description looks a bit clunky here: 
|
|
Flaky tests detected in 4ab6b5c0412ab1efd0e998993b390b3e2cc61773. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7007381730
|
I updated the |
|
Thanks for that Nik, I'm going to eat my own words though – in the context of the whole view I'm wondering if it could be confusing that some templates have descriptions and others not. It's probably clearer to include the description even if empty. We can use a I think the layout of the fields is in an okay spot for now. It's still not perfect, but until we expand the API and/or work on specific views I don't think we can go too much further. There are just a couple of outstanding details to address before we can merge:
|
ntsekouras
force-pushed
the
refine/dataviews-grid
branch
from
6f1be22 to
f06fc67
Compare
Is this wanted for the placeholder too? Additionally if we want this for every data view through
This is done. |
Yeah I think the behavior should be consistent regardless of whether a media file / preview is found. It's hard to say for certain, but except for the title and the media item I don't think we'd want any other fields to act as a link this way.
Nice 🚀 |
|
One last thing to fix, in the view options the 'Preview' item doesn't do anything: 
I suppose we should treat it the same as the title and hide the option when grid layout is selected. What do you think? |
This is the same issue with pages and is tracked in the main issue. We can do it separately. |
|
Ok. Once the media item is hooked up as a link I think we can merge. |
I think this will need more thought and exploration of how to handle it best, because the component will be a generic one and could show any items, without necessarily having a separate We can create a new issue or a task in the main issue to track it. |
|
This will be interesting to take a closer look at in the Gutenberg plugin next week! |
* Dataviews: Grid layout refinements * grid compact item actions * Revert tooltips and update styles * temp fix to not render empty values in grid * linting issues * do not render description in grid view if empty * add theme's background color in previews empty space --------- Co-authored-by: James Koster <[email protected]>
What?
Part of: #55733
This PR includes some Grid layout refinements. This is the starting point and designer input and polishing is needed.
It will be better to test when we land the preview in templates list
.
Testing Instructions