DataViews: Update all templates page
#55848
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
ntsekouras
added
[Type] Experimental
ntsekouras
requested review from
mcsf,
youknowriad,
oandregal,
jameskoster,
SaxonF and
jorgefilipecosta
|
Size Change: -884 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d9cb65d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6799852261
|
youknowriad
reviewed
Nov 6, 2023
There was a problem hiding this comment.
Nice PR. I'm curious? How was the experience using the data views to build this? Maybe there should be a way to disable pagination entirely for a view or maybe we should start with more than 10 templates per page.
Also, let's get some design feedback on this. @SaxonF @jameskoster @jasmussen
packages/edit-site/src/components/page-templates/dataviews-templates.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/page-templates/dataviews-templates.js
Outdated
Show resolved
Hide resolved
|
Exciting! Grid view is going to be a little tricky here as templates do not have a featured image. Ideally we show a preview like we do for Patterns, but I appreciate that might be tricky in terms of performance. So to begin with maybe we disable the grid view? Hiding pagination when there's only a single page like Riad suggested makes sense. Also agree that 5 isn't a very useful option there. 10 could be a good minimum. 20 could be the default (like in wp-admin). This might be worth looking at separately though because we need to think about the interaction with grid view. Specifically; it would be nice to avoid 'orphan' grid items. Relating to the Templates list directly:
Noting also that we need to settle on general styles for spacing, typography, color etc. But I think that's one to handle in a dedicated PR. |
ntsekouras
force-pushed
the
add/dataviews-templates-page
branch
from
36e8fff
to
ecc788f
Compare
oandregal
reviewed
Nov 8, 2023
packages/edit-site/src/components/page-templates/dataviews-templates.js
Outdated
Show resolved
Hide resolved
oandregal
reviewed
Nov 8, 2023
packages/edit-site/src/components/page-templates/dataviews-templates.js
Outdated
Show resolved
Hide resolved
oandregal
reviewed
Nov 8, 2023
packages/edit-site/src/components/page-templates/dataviews-templates.js
Outdated
Show resolved
Hide resolved
oandregal
reviewed
Nov 8, 2023
packages/edit-site/src/components/page-templates/dataviews-templates.js
Outdated
Show resolved
Hide resolved
ntsekouras
force-pushed
the
add/dataviews-templates-page
branch
from
1fd196c
to
7b62907
Compare
oandregal
reviewed
Nov 8, 2023
oandregal
reviewed
Nov 8, 2023
ntsekouras
force-pushed
the
add/dataviews-templates-page
branch
from
7b62907
to
885cdea
Compare
@youknowriad pretty straightforward so far. The main difference for now is that the data are handled with a single REST request, so we have to handle the data changes locally. The list hasn't filters yet, so I'm not sure what we will face later - I'd expect no big surprises though. The other thing that was surfaced here and will probably need to handle better with more thought in follow ups, is that the component might need to support specific view types. @jameskoster I haven't addressed the following:
I think we have feature parity for now, and we could handle all these separately in follow ups, so work could be even split among more people. So what do you think is missing or needs to be addressed to land the first iteration of this? |
What do you mean by this? |
oandregal
reviewed
Nov 8, 2023
| } ); | ||
| } | ||
| // Handle sorting. | ||
| // TODO: Explore how this can be more dynamic.. |
There was a problem hiding this comment.
What do you mean by this comment?
There was a problem hiding this comment.
Not really sure yet 😄 . We'll know when we add more sortable fields soon. Might not need anything special.
I'm not sure how we will handle it in the future based on allowing third party devs to register their own view types etc.. I don't have something specific in my mind now, but we might need some different handling than the |
oandregal
reviewed
Nov 8, 2023
| @@ -44,8 +44,17 @@ const availableViews = [ | |||
| }, | |||
| ]; | |||
|
|
|||
| function ViewTypeMenu( { view, onChangeView } ) { | |||
| const activeView = availableViews.find( ( v ) => view.type === v.id ); | |||
| function ViewTypeMenu( { view, onChangeView, supportedViewTypes } ) { | |||
There was a problem hiding this comment.
I only have naming suggestions:
- Using "layout" instead of "view type" everywhere:
- We expose it as "Layout" in the UI, so it's better to be coherent with that – it also communicates intent better than type, IMO.
- Some changes would be:
ViewTypeMenutoLayoutMenu,availableViewstoavailableLayouts, etc. - I understand there's previous code, so unrelated changes could be done separately, not in this PR.
supportedViewTypesprop tolayouts. Introduced in this PR, so we can do it here already.
oandregal
reviewed
Nov 8, 2023
| const activeView = availableViews.find( ( v ) => view.type === v.id ); | ||
| function ViewTypeMenu( { view, onChangeView, supportedViewTypes } ) { | ||
| let _availableViews = availableViews; | ||
| if ( supportedViewTypes ) { |
There was a problem hiding this comment.
What do you think of reusing the same array as in?
if ( layouts ) {
avaliableLayouts = availableLayouts.filter( ... );
}An alternative would be to do the following name changes:
availableViews=>layouts_availableViews=>availableLayouts
There was a problem hiding this comment.
I actually like the alternative naming best (layouts/availableLayouts).
There was a problem hiding this comment.
Ah, but then it conflicts with the prop. Perhaps?
let availableLayouts = defaultLayouts;
if ( layouts ) {
availableLayouts = defaultLayouts.filter( ... );
}There was a problem hiding this comment.
We do have view type in some places style, we might need to think that properly. I'm not entirely sure "layout" is the right word in the "code" at least. For user facing things it's fine. But I kind of like the "block" / "block type" and "view" / "view type" parallel.
Anyway, we need to make a decision and stick with it but it's probably one for a separate PR.
oandregal
reviewed
Nov 8, 2023
packages/edit-site/src/components/page-templates/template-actions.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Nov 9, 2023
packages/edit-site/src/components/page-templates/template-actions.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Nov 9, 2023
oandregal
reviewed
Nov 9, 2023
oandregal
reviewed
Nov 9, 2023
packages/edit-site/src/components/page-templates/dataviews-templates.js
Outdated
Show resolved
Hide resolved
|
Would appreciate a second review but this is looking good for v1 for me. |
oandregal
approved these changes
Nov 9, 2023
This was referenced Nov 10, 2023
cbravobernal
pushed a commit
that referenced
this pull request
Nov 14, 2023
* [Data views]: Update `all templates` page * feedback part 1 * simple remove action * Allow actions to use a Modal * support only list view for first iteration * remove leftovers since only supporting list view type * refactor item actions * fix action item on click callback
oandregal
changed the title
all templates pageall templates page
This was referenced Nov 20, 2023
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.
What?
Part of: #55083
This PR updates the templates list in site editor and is under new admin views experiment.
In order to implement the first version of this list, data views actions API had to be augmented to enable actions to render content in a Modal(ex. confirmation for deleting a template).
Testing Instructions
manage all templatesviewTasks
listview type for first iterationScreenshots or screencast
Screen.Recording.2023-11-09.at.10.15.52.PM.mov