DataViews: add list layout to templates #57014
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oandregal
added
[Type] Experimental
oandregal
requested review from
ntsekouras,
jorgefilipecosta,
youknowriad and
jameskoster
youknowriad
reviewed
Dec 13, 2023
| @@ -46,6 +47,7 @@ import { | |||
| } from './template-actions'; | |||
| import usePatternSettings from '../page-patterns/use-pattern-settings'; | |||
| import { unlock } from '../../lock-unlock'; | |||
| import SideEditor from '../page-pages/side-editor'; | |||
There was a problem hiding this comment.
It's a bit weird that the "templates" page is importing from the "pages" page.
There was a problem hiding this comment.
What's a good place to have the SideEditor component, so both pages can reuse it?
There was a problem hiding this comment.
I think it should just be in its own folder under components. Maybe <PostPreview postId={postId} postType={postType} />. That component shouldn't be an "editor". It's just a preview of the current selected entity.
To be honest, that component as written right now is a hack. The fact that it works is just random. For instance, it's impossible to render two of these components on a page. I'd add a comment to the component to explicitly mention that it's hacky and that it shouldn't be relied on as is forever.
There was a problem hiding this comment.
Though, we also have import usePatternSettings from '../page-patterns/use-pattern-settings'; that could find a better place as well.
There was a problem hiding this comment.
This one should probably be in its own folder as well.
There was a problem hiding this comment.
Extracted SideEditor to its own PostPreview component.
|
There's #56983 to make the filters UI footprint smaller. I'd think both can land independently, even if it means this PR lands first and the filters take too much space. |
|
The obvious issue here which is similar to the "pages" list view as well, is that the canvas should be a single inert button and when clicking on the canvas, we should navigate to the "edit template" page with the sidebar closed. |
|
Size Change: +110 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Also there's a weird effect where if I change the layout the position of the "view" button changes, meaning the dropdown stays open but it's weirdly positioned. Should we close the dropdown menu when switching layouts? |
Fix for that at #57015 |
|
Flaky tests detected in 99f400b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7195538117
|
Yeah, I'd like us to investigate that separately. It's listed as a follow-up task in the list layout at #55083 I'll look into that soon if no one else does it first. |
|
Pushed a couple of small style tweaks. The template description is a bit awkward in this view, especially as it needs to be truncated, or when there is no description (custom templates). Would it be feasible to disable that field in this view? Similarly the author prominence seems a bit outsized in this view. Hiding the icon could help a bit. 
Finally – and this might be separate – but we can probably omit the "Customized" label on theme templates that have been edited. |
ntsekouras
reviewed
Dec 13, 2023
| @@ -59,6 +61,10 @@ const defaultConfigPerViewType = { | |||
| mediaField: 'preview', | |||
| primaryField: 'title', | |||
| }, | |||
| [ LAYOUT_LIST ]: { | |||
| primaryField: 'title', | |||
There was a problem hiding this comment.
Do you think the primaryField could be different between views? I can't think of a use case like that, so maybe we should add in the fields API(ex isPrimary)? 🤔
There was a problem hiding this comment.
maybe it can be modified, and in this case, it just becomes a top level config, like "sort" or "visibleFields" or something like that.
There was a problem hiding this comment.
Do you think the primaryField could be different between views? I can't think of a use case like that, so maybe we should add in the fields API(ex isPrimary)? 🤔
I feel the same as you: it's quite improbable that it's different across views. However, I think the meaning of "primary" is something that belongs to the view API and not the field API.
maybe it can be modified, and in this case, it just becomes a top level config, like "sort" or "visibleFields" or something like that.
By the user, you mean?
There was a problem hiding this comment.
user or the person that creates the view, in which case it's in the view object like you said
There was a problem hiding this comment.
I'd leave it as it is in this PR. If it requires further action, it can be done separately.
oandregal
force-pushed
the
add/list-view-to-templates
branch
from
1adf02e
to
99f400b
Compare
Would you mind if we do this in a follow-up? I'd like to revisit this together with the
I've done this.
I've removed the customized label from all layouts. |
youknowriad
approved these changes
Dec 13, 2023
artemiomorales
pushed a commit
that referenced
this pull request
Jan 4, 2024
Co-authored-by: James Koster <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #55083
What?
Adds the list layout to the templates page.
There's #56983 to make the filters UI footprint smaller. I'd think both can land independently, even if it means this PR lands first and the filters take too much space.
Gravacao.do.ecra.2023-12-13.as.11.32.41.mov
Why?
We want the list layout to become the main navigation screen for templates.
How?
Implements the list layout for templates:
primaryFieldandmediaFieldTesting Instructions