DataViews: Unify layout config #67477
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
force-pushed
the
unify/dataviews-layout
branch
from
978e283 to
7f80b9d
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
On the template list view we allow to hide and show the preview, but showing just adds an empty space. Why do we allow to hide and show the preview on list view but not on the other layouts? I think for now the ability to hide and show media fields was not yet implemented.
|
@jorgefilipecosta Because, I wanted to try the possibility to have a "mediaField" defined in one layout but not others (to check that it works). We can define it in all views and the "preview" won't be available to show/hide anymore. That said, I want to first introduce the possibility to toggle these fields visibility on/off first. |
|
Size Change: -354 B (-0.02%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
This is getting very close to be ready (missing the table header), I would appreciate more design reviews... |
|
There's something not right with the properties list in view options; the UI around toggling visibility is behaving unexpectedly. Probably best described with a video: visibility.mp4FWIW if this could be made into a single list, rather than separating visible / invisible properties that would might be a win. On the Pages dataview the Title appears separate from other properties, and I'm unable to re-order the first item in the list: pages.mp4Patterns seems okay, though I notice there's no set order by default. That's the case on trunk too, but it may be a bug? |
It's important to note that the three first fields are special, in the sense that they can't be moved up or down. I can make it look like a single list but it's not a single list, you can move the "author" above "media" or media above title.
This is by design, these fields are special and can't be moved (see the issue) |
|
Understood. It's most confusing in the Templates example, where toggling Author visibility appears to just remove the 'Hidden' label. I think the list needs a bit of design work to communicate the different concepts. Leave it with me. |
|
Actually...
In the pages case 'Title' is the only special field, correct? Why can't I move the first property in the list beneath title? |
Title and featured image are the special fields in the "pages" case. You should be able to move "status", maybe it's a bug there. I'll take a look. |
|
@jameskoster Fixed the bug and it looks like there's no featured image for pages, I was wrong there :P |
A quick mockup for this one: list.mp4
This maintains the appearance of a single list while (hopefully) communicating the concepts at play a bit better. I'd welcome feedback though cc @WordPress/gutenberg-design |
The problem here is that at the moment we can't keep them at the same position, we can move them at the end of the list if needed but their "previous" order is lost when they're hidden. |
| const viewFields = view.fields || fields.map( ( field ) => field.id ); | ||
| const { visibleFields, badgeFields } = fields.reduce( | ||
| const otherFields = view.fields ?? []; | ||
| const { regularFields, badgeFields } = fields.reduce( |
There was a problem hiding this comment.
Could this benefit from a useMemo to avoid creating new objects on every rerender?
There was a problem hiding this comment.
It would only benefit from useMemo if the the component receiving this prop is memoized. I'd avoid this kind of early optimizations for now because we're not measuring that this is a perf issue but it could be something to explore once we have better dataviews performance tracking.
| */ | ||
| direction: 'horizontal' | 'vertical'; | ||
| showMedia?: boolean; |
There was a problem hiding this comment.
Could we remove the showMedia Property and assume that if mediaField is not on the list of visible fields it is hidden?
Related soon we would have the ability to offer multiple media fields and switch between them #67278.
The way I see things in the future is mediaField(s) specified which fields can be media e.g: "featured-image", "preview" etc. And in fields we specify which media fields are visible (in some layouts like grid only one is visible in other like table multiple ones can be visible).
There was a problem hiding this comment.
Could we remove the showMedia Property and assume that if mediaField is not on the list of visible fields it is hidden?
If we do this, the "reordering" of the fields (other than media, title, description) becomes more complex, we'd need to "skip" them kind of regardless of where they are in the array.
There was a problem hiding this comment.
I guess it is ok to keep showMedia for now but on #67278 we may need to do changes.
There was a problem hiding this comment.
Why would we change it?
There was a problem hiding this comment.
We will have multiple media fields, I guess the way which we use to say which of the media fields is active could be used to say no media field is active at all.
There was a problem hiding this comment.
I think these two things might be separate things. "picking a media field" and "toggling its visibility". We have mediaField property in the view for picking the mediaField and showMedia is for whether to show it or not.
There was a problem hiding this comment.
The plan was for mediaField to become mediaFields to indicate the fields that could be media. We could say the first array item is the active one.
One thing we are not able to express would be to allow multiple media fields to be shown on the table layout, but I guess that's okay; otherwise, we would have different behaviors between the layouts.
There was a problem hiding this comment.
I think the active media field and the available media fields are two different things. For the latter I'm not sure it's something for the "view" object and more a config of the fields themselves.
There was a problem hiding this comment.
For the latter I'm not sure it's something for the "view" object and more a config of the fields themselves.
We were discussing that at #67278 (comment).
We can continue the discussion there both options are a possibility either the field it self says it is media, or the view says which fields are media. I guess we need to decide.
Okay, let's tweak things a bit: properties.mp4Hidden fields move to the end of the list, with a stronger separator (same as special fields). No ordering controls appear on hover. Do you think this could work? |
Yes, personally I don't really like the strong separator (I barely see it) but yes it can work. |
|
@jameskoster I'm finding it a bit confusing to mix "hidden" and "visible" fields in the same list (when trying this). I'll push in a bit but wanted to let you know. |
youknowriad
force-pushed
the
unify/dataviews-layout
branch
from
8a1e4fd to
e6f61cd
Compare
|
Pending the question on "show" or "hide" for the configs, I think I addressed all your comments @oandregal |
Did you push this, it doesn't look like the mockups?
|
|
@jameskoster I guess I updated the behavior but not these small details :) |
|
Made these tweaks, I'm still not satisfied fully personally. I'd rather show these fields in a separate list because they are separate, they are special and not rendered in the same list as the rest. That said, I'm fine with this as a first iteration. I think we need to get this PR in and we can always iterate on the details of how the menu should look. |
packages/dataviews/src/components/dataviews-view-config/style.scss
Outdated
Show resolved
Hide resolved
|
Checking empty states. In theory, consumers can configure a layout without any of the primary/media/description fields. This is how it'd look like:
Some thoughts:
|
|
I think we should
But I think we should do all these adaptations in follow-ups. |
| mediaField: 'featured_media', | ||
| }, | ||
| }, | ||
| [ LAYOUT_TABLE ]: {}, |
There was a problem hiding this comment.
This PR de-emphasizes what defaultLayouts was about. We could make it optional now, as the only cases where consumers would pass this are: 1) they want to disable a specific layout 2) configure grid (badges, or default preview size), or 3) configure table (styles, or default density)
There was a problem hiding this comment.
It's also about providing the available layouts to switch to. omitting it means no layout switching.
There was a problem hiding this comment.
I agree we should de-emphasize even more. "badge" should be cross layout.
I'm not sure how to solve the rest (preview size, styles, default density)
There was a problem hiding this comment.
Epiphany: I feel we may have been thinking about this wrong :)
Instead of trying to guess how to switch layouts within DataViews, it seems the right solution is to actually remove the view type switching logic from the DataViews component itself. It should be just injected as an external component (in header prop maybe)
There was a problem hiding this comment.
I'm not sure if that's what you're thinking about, but extending layouts is relevant to this conversation. I think the final API could be something along these lines:
<DataViews
layouts=[ 'table', { type: 'grid', badgeFields: [ 'field' ] }, MyCustomLayoutComponent ]
/>which means:
- use the bundled
tablelayout as it is - use the bundled
gridlayout, and configure the following fields as badge fields - do not use the bundled
listlayout, because it's not part of the array - Use the
MyCustomLayoutComponentlayout, a custom not bundled layout that implements everything a layout should have (icon & name for the switcher, behaviour, any view config control, etc.)
| <HStack | ||
| justify="flex-end" | ||
| expanded={ false } | ||
| className="dataviews-field-control__actions" | ||
| > | ||
| { view.type === LAYOUT_TABLE && isVisible && ( | ||
| { isVisible && ( |
There was a problem hiding this comment.
I think moving fields could work in grid & list layouts, so leaving those controls visible would be good. Though, right now, they don't work, so this is what happens:
Screen.Recording.2024-12-05.at.18.48.27.mov
If we implement moving the fields in list&grid layouts, it helps us making the view config layout-agnostic. That's a win.
There was a problem hiding this comment.
I don't think we should allow moving the special fields in either layout, specifically to stay consistent. In "table" it doesn't make sense to put "media" after "title" for instance (same for list I think). So instead of trying to guess what can work where. It's better to be opinionated here. This is the main goal of this PR.
There was a problem hiding this comment.
I don't think we should allow moving the special fields in either layout, specifically to stay consistent. In "table" it doesn't make sense to put "media" after "title" for instance (same for list I think). So instead of trying to guess what can work where. It's better to be opinionated here. This is the main goal of this PR.
I agree and that's not what I suggested :) My point was about the other fields: in list & grid the moving controls are visible but they do nothing to the layout. Either we make them invisible (like trunk) or update the layout to actually move the fields (and I prefer the later).
There was a problem hiding this comment.
moving controls are visible but they do nothing to the layout.
Oh I didn't know that. I just assumed that they worked because it was "obvious" to me that they should work :P
Happy to address that in a follow-up.
|
Thank you for adjusting those details @youknowriad. I agree this isn't perfect but it's in a decent spot. I'll open an issue about communicating the 'special' nature of fields in the design. |
Fixes #67391
What?
As explained in the main issue, the purpose of this PR is to unify the layout config and behavior for title, media and description fields. For the full reasoning, check the issue.
This is still not finished, but here's a todo list to track the progress. It's already possible to review I think.
Todo
Testing Instructions
1- Open the different dataviews (templates, patterns and pages)
2- Check the design of the different layouts.
3- Check how the "properties" menu work in the different layout.