DataViews: Add sync filter in patterns page #57532
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 and
SaxonF
|
Size Change: +316 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jan 4, 2024
| _fields.push( { | ||
| header: __( 'Sync Status' ), | ||
| id: 'sync-status', | ||
| isVirtual: true, |
There was a problem hiding this comment.
Why should this be a virtual filter. For me it's actually a valid field that can show Sync or not.
There was a problem hiding this comment.
We could definitely delay this virtual concept for now, and by not providing a render prop and adjusting the views to check for that, we could not display the field. My exploration was based on thoughts I had for other cases too, as I describe in the PR's description.
There was a problem hiding this comment.
To be honest, I can't think of a case where a filter doesn't make sense as a regular field personally.
youknowriad
reviewed
Jan 4, 2024
youknowriad
reviewed
Jan 4, 2024
youknowriad
reviewed
Jan 4, 2024
ntsekouras
force-pushed
the
dataviews/add-patterns-sync-filter
branch
from
41af9ae
to
b86cbd2
Compare
oandregal
reviewed
Jan 4, 2024
oandregal
reviewed
Jan 4, 2024
packages/edit-site/src/components/page-patterns/dataviews-patterns.js
Outdated
Show resolved
Hide resolved
oandregal
reviewed
Jan 4, 2024
| [ view.type, categoryId ] | ||
| ); | ||
| ]; | ||
| if ( type === PATTERN_TYPES.theme ) { |
There was a problem hiding this comment.
How do I trigger a situation in which the category type is PATTERN_TYPES.user?
There was a problem hiding this comment.
To test
- I went to the "manage my patterns" screen
- edited one of my patterns by adding a new "howdy" category
I see this new category is listed in the patterns page, though it works as any other:
Gravacao.do.ecra.2024-01-04.as.14.27.03.mov
Should I do anything differently?
There was a problem hiding this comment.
This is for later, but making sync visible and changing the listing hide the field (this is: the sidebar is not using the views yet, like the pages page).
Gravacao.do.ecra.2024-01-04.as.14.32.12.mov
Though, this makes me think that I wouldn't want the sync field to be removed from the view. Consider the following case: a user is navigating through the categories to see what patterns are available, though they are only interested in "not synced" patterns:
syncfield is shown in the layoutsyncfilter is also shown
What happens when the user visits a user-provided category and then a theme provided category?
The consequences are that 1) visual jumps in the layout and 2) the user preferences (hiddenFields and filter statuses) are cleared once they visit a user-provided category.
It sounds it's to always render the sync field and its filter.
There was a problem hiding this comment.
How do I trigger a situation in which the category type is PATTERN_TYPES.user?
Did you encountered something unexpected that is different from the old patterns page?
I'm not really sure but that param contains the value wp_block. Maybe @talldan or @aaronrobertshaw can shed some light here?
This is for later, but making sync visible and changing the listing hide the field (this is: the sidebar is not using the views yet, like the pages page).
We discussed this a bit with Riad here and for now we reset the whole view. It does resemble the custom views we have in pages list. We can revisit later if we need to..
There was a problem hiding this comment.
Did you encountered something unexpected that is different from the old patterns page?
I don't know, because I don't know how to trigger a situation where type is not PATTERN_TYPES.thems :)
From what I gathered, my suggestion here is to always render the sync-status field and its filter.
There was a problem hiding this comment.
Taking a look. What's confusing is there are two PATTERN_TYPES consts that seem to have different values.
One is used in the inserter for inserter items:
One is used by most of the other patterns code:
gutenberg/packages/patterns/src/constants.js
Lines 6 to 9 in 6f61d8b
There was a problem hiding this comment.
It seems the majority of the codebase uses the first variable, where PATTERN_TYPES.user === 'user'.
It seems you're using the second variable where PATTERN_TYPES.user === 'wp_block', it doesn't seem to be used much in the codebase.
I think the first variable is the one you'll want to use, though I think it's still not great as the one object seems to mix and match lots of different concepts (syncing, pattern source, and I'm not sure what 'all' is).
Ignore that, my assumption was wrong.
This definitely needs some improvement!
There was a problem hiding this comment.
So I think sometimes we're working with one kind of pattern item (where type is user) and other times a different type of item (where type is wp_block).
I've started looking into improving the code quality. First PR here - Rename patternBlock to patternPost
I've made another PR that renames the inserter constants to better indicate that they're for inserter items - Code Quality: Improve inserter pattern constants.
I think the goal should be to have a single normalized pattern item data structure (even if the data is sourced from different places) with consistent values, then hopefully we'll be able to tidy up the constants.
There was a problem hiding this comment.
I think I have a full understanding of what the code is doing now.
There's a categoryType sprinkled across the codebase, but this is purely for routing and can be one of two values, pattern (for any time of pattern) or wp_template_part (for template parts). It's confusing that PATTERN_TYPES is often used to check this in addition to being used to check the pattern item types. The category type values are also unusual with one being a post type and the other not.
I'll follow up with another PR that improves this, but it'll be next week.
There was a problem hiding this comment.
Nice, thanks for looking into this, Dan.
oandregal
approved these changes
Jan 4, 2024
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?
This PR adds a
sync statusfilter in patterns page.This PR does a couple of more small things:
enumerationto provide a helping descriptionDesign wise
The old patterns page adds an extra header when we apply one of the
sync statusfilter. For now I just added that description in the filter menu item, as a helper text.Testing Instructions
sync statusfilters works as expectedsync statusfield is not visible in sorting and hideable fields menusScreenshots or screencast
Screen.Recording.2024-01-04.at.8.49.35.AM.mov