Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DataViews: Add sync filter in patterns page #57532

Merged
merged 4 commits into from Jan 4, 2024
Merged

DataViews: Add sync filter in patterns page #57532

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
252a076
DataViews: Add sync filter in patterns page
ntsekouras Jan 4, 2024
a8a3ce3
remove isVirtual field
ntsekouras Jan 4, 2024
b86cbd2
display `sync status` field
ntsekouras Jan 4, 2024
5ed07e9
minor tweak
ntsekouras Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/dataviews/src/add-filter.js
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ const {
DropdownMenuRadioItemV2: DropdownMenuRadioItem,
DropdownMenuSeparatorV2: DropdownMenuSeparator,
DropdownMenuItemLabelV2: DropdownMenuItemLabel,
DropdownMenuItemHelpTextV2: DropdownMenuItemHelpText,
} = unlock( componentsPrivateApis );

function WithSeparators( { children } ) {
Expand Down Expand Up @@ -143,6 +144,13 @@ export default function AddFilter( { filters, view, onChangeView } ) {
<DropdownMenuItemLabel>
{ element.label }
</DropdownMenuItemLabel>
{ !! element.description && (
<DropdownMenuItemHelpText>
{
element.description
}
</DropdownMenuItemHelpText>
) }
</DropdownMenuRadioItemCustom>
);
} ) }
Expand Down
6 changes: 6 additions & 0 deletions packages/dataviews/src/filter-summary.js
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ const {
DropdownMenuItemV2: DropdownMenuItem,
DropdownMenuSeparatorV2: DropdownMenuSeparator,
DropdownMenuItemLabelV2: DropdownMenuItemLabel,
DropdownMenuItemHelpTextV2: DropdownMenuItemHelpText,
} = unlock( componentsPrivateApis );

const FilterText = ( { activeElement, filterInView, filter } ) => {
Expand Down Expand Up @@ -126,6 +127,11 @@ export default function FilterSummary( { filter, view, onChangeView } ) {
<DropdownMenuItemLabel>
{ element.label }
</DropdownMenuItemLabel>
{ !! element.description && (
<DropdownMenuItemHelpText>
{ element.description }
</DropdownMenuItemHelpText>
) }
</DropdownMenuRadioItemCustom>
);
} ) }
Expand Down
5 changes: 4 additions & 1 deletion packages/dataviews/src/filters.js
View file
Open in desktop
Expand Up @@ -40,10 +40,13 @@ const Filters = memo( function Filters( { fields, view, onChangeView } ) {

switch ( field.type ) {
case ENUMERATION_TYPE:
if ( ! field.elements?.length ) {
return;
}
filters.push( {
field: field.id,
name: field.header,
elements: field.elements || [],
elements: field.elements,
operators,
isVisible: view.filters.some(
( f ) =>
Expand Down
1 change: 1 addition & 0 deletions packages/dataviews/src/view-grid.js
View file
Open in desktop
Expand Up @@ -30,6 +30,7 @@ export default function ViewGrid( {
);
const visibleFields = fields.filter(
( field ) =>
!! field.render &&
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
! view.hiddenFields.includes( field.id ) &&
! [ view.layout.mediaField, view.layout.primaryField ].includes(
field.id
Expand Down
1 change: 1 addition & 0 deletions packages/dataviews/src/view-list.js
View file
Open in desktop
Expand Up @@ -32,6 +32,7 @@ export default function ViewList( {
);
const visibleFields = fields.filter(
( field ) =>
!! field.render &&
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
! view.hiddenFields.includes( field.id ) &&
! [ view.layout.primaryField, view.layout.mediaField ].includes(
field.id
Expand Down
7 changes: 7 additions & 0 deletions packages/dataviews/src/view-table.js
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ const {
DropdownMenuRadioItemV2: DropdownMenuRadioItem,
DropdownMenuSeparatorV2: DropdownMenuSeparator,
DropdownMenuItemLabelV2: DropdownMenuItemLabel,
DropdownMenuItemHelpTextV2: DropdownMenuItemHelpText,
} = unlock( componentsPrivateApis );

const sortArrows = { asc: '↑', desc: '↓' };
Expand Down Expand Up @@ -200,6 +201,11 @@ function HeaderMenu( { field, view, onChangeView } ) {
<DropdownMenuItemLabel>
{ element.label }
</DropdownMenuItemLabel>
{ !! element.description && (
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
<DropdownMenuItemHelpText>
{ element.description }
</DropdownMenuItemHelpText>
) }
</DropdownMenuRadioItemCustom>
);
} ) }
Expand Down Expand Up @@ -294,6 +300,7 @@ function ViewTable( {
} ) {
const visibleFields = fields.filter(
( field ) =>
!! field.render &&
! view.hiddenFields.includes( field.id ) &&
! [ view.layout.mediaField, view.layout.primaryField ].includes(
field.id
Expand Down
58 changes: 44 additions & 14 deletions packages/edit-site/src/components/page-patterns/dataviews-patterns.js
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@ import {
Flex,
} from '@wordpress/components';
import { getQueryArgs } from '@wordpress/url';
import { __ } from '@wordpress/i18n';
import { __, _x } from '@wordpress/i18n';
import {
useState,
useMemo,
Expand Down Expand Up @@ -46,6 +46,8 @@ import {
TEMPLATE_PART_POST_TYPE,
PATTERN_SYNC_TYPES,
PATTERN_DEFAULT_CATEGORY,
ENUMERATION_TYPE,
OPERATOR_IN,
} from '../../utils/constants';
import {
exportJSONaction,
Expand Down Expand Up @@ -83,6 +85,24 @@ const DEFAULT_VIEW = {
filters: [],
};

const SYNC_FILTERS = [
{
value: PATTERN_SYNC_TYPES.full,
label: _x( 'Synced', 'Option that shows all synchronized patterns' ),
description: __( 'Patterns that are kept in sync across the site.' ),
},
{
value: PATTERN_SYNC_TYPES.unsynced,
label: _x(
'Not synced',
'Option that shows all patterns that are not synchronized'
),
description: __(
'Patterns that can be changed freely without affecting the site.'
),
},
];

function Preview( { item, viewType } ) {
const descriptionId = useId();
const isUserPattern = item.type === PATTERN_TYPES.user;
Expand Down Expand Up @@ -204,27 +224,25 @@ export default function DataviewsPatterns() {
const isUncategorizedThemePatterns =
type === PATTERN_TYPES.theme && categoryId === 'uncategorized';
const previousCategoryId = usePrevious( categoryId );
const viewSyncStatus = view.filters?.find(
( { field } ) => field === 'sync-status'
)?.value;
const { patterns, isResolving } = usePatterns(
type,
isUncategorizedThemePatterns ? '' : categoryId,
{
search: view.search,
// syncStatus:
// deferredSyncedFilter === 'all'
// ? undefined
// : deferredSyncedFilter,
syncStatus: viewSyncStatus,
}
);
const fields = useMemo(
() => [
const fields = useMemo( () => {
const _fields = [
{
header: __( 'Preview' ),
id: 'preview',
render: ( { item } ) => (
<Preview item={ item } viewType={ view.type } />
),
minWidth: 120,
maxWidth: 120,
enableSorting: false,
enableHiding: false,
},
Expand All @@ -235,12 +253,24 @@ export default function DataviewsPatterns() {
render: ( { item } ) => (
<Title item={ item } categoryId={ categoryId } />
),
maxWidth: 400,
enableHiding: false,
},
],
[ view.type, categoryId ]
);
];
if ( type === PATTERN_TYPES.theme ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I trigger a situation in which the category type is PATTERN_TYPES.user?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • sync field is shown in the layout
  • sync filter is also shown
Captura de ecrã 2024-01-04, às 14 39 48

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.

Copy link
Contributor Author

@ntsekouras ntsekouras Jan 4, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@talldan talldan Jan 5, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

gutenberg/packages/block-editor/src/components/inserter/block-patterns-tab/utils.js

Lines 7 to 14 in 6f61d8b

export const PATTERN_TYPES = {
all: 'all',
synced: 'synced',
unsynced: 'unsynced',
user: 'user',
theme: 'theme',
directory: 'directory',
};

One is used by most of the other patterns code:

gutenberg/packages/patterns/src/constants.js

Lines 6 to 9 in 6f61d8b

export const PATTERN_TYPES = {
theme: 'pattern',
user: 'wp_block',
};

Copy link
Contributor

@talldan talldan Jan 5, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

@talldan talldan Jan 5, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for looking into this, Dan.

_fields.push( {
header: __( 'Sync Status' ),
id: 'sync-status',
type: ENUMERATION_TYPE,
elements: SYNC_FILTERS,
filterBy: {
operators: [ OPERATOR_IN ],
},
enableSorting: false,
enableHiding: false,
} );
}
return _fields;
}, [ view.type, categoryId, type ] );
// Reset the page number when the category changes.
useEffect( () => {
if ( previousCategoryId !== categoryId ) {
Expand Down Expand Up @@ -318,7 +348,7 @@ export default function DataviewsPatterns() {
view={ view }
onChangeView={ onChangeView }
deferredRendering={ true }
supportedLayouts={ [ LAYOUT_GRID ] }
supportedLayouts={ [ LAYOUT_GRID, 'table' ] }
/>
</Page>
</ExperimentalBlockEditorProvider>
Expand Down