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

Patterns: Add category selector to pattern creation modal #55024

Merged
merged 17 commits into from
Oct 9, 2023
Merged

Patterns: Add category selector to pattern creation modal #55024

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3cd8056
Add category selector to pattern creation modal
glendaviesnz Oct 3, 2023
cffbb95
Switch to adding categories to the form token suggestion list
glendaviesnz Oct 4, 2023
bb764fe
tidy up variable naming
glendaviesnz Oct 4, 2023
774ade4
Make sure the slugs used in taxonomy match those used in core pattern…
glendaviesnz Oct 4, 2023
376efef
Get categories from core store
glendaviesnz Oct 4, 2023
ee676d6
Fix label
glendaviesnz Oct 4, 2023
232772a
Simplify the merging of the categories.
glendaviesnz Oct 4, 2023
3945d70
Explain removal of the query slug category
glendaviesnz Oct 4, 2023
3817dbe
fix comment typo
glendaviesnz Oct 4, 2023
18c49a4
Switch to using a map instead of an array for storing categories
glendaviesnz Oct 5, 2023
01bca09
switch to map has instead of get
glendaviesnz Oct 5, 2023
a200196
Use a forEach instead of a reduce
glendaviesnz Oct 5, 2023
7815d52
Fix typo
glendaviesnz Oct 5, 2023
29865fb
Remove unneeded fallback array
glendaviesnz Oct 5, 2023
ba3c8e1
Remove redundant `true` prop values
glendaviesnz Oct 5, 2023
906df26
Fix dynamic height and width when adding new categories
kevin940726 Oct 6, 2023
862078f
Fix suggestions list width
kevin940726 Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 27 additions & 42 deletions packages/patterns/src/components/category-selector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,49 +4,36 @@
import { __ } from '@wordpress/i18n';
import { useMemo, useState } from '@wordpress/element';
import { FormTokenField } from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import { store as coreStore } from '@wordpress/core-data';
import { useDebounce } from '@wordpress/compose';
import { decodeEntities } from '@wordpress/html-entities';

const unescapeString = ( arg ) => {
return decodeEntities( arg );
};

const EMPTY_ARRAY = [];
const MAX_TERMS_SUGGESTIONS = 20;
const DEFAULT_QUERY = {
per_page: MAX_TERMS_SUGGESTIONS,
_fields: 'id,name',
context: 'view',
};
export const CATEGORY_SLUG = 'wp_pattern_category';

export default function CategorySelector( { values, onChange } ) {
export default function CategorySelector( {
categoryTerms,
onChange,
categoryMap,
} ) {
const [ search, setSearch ] = useState( '' );
const debouncedSearch = useDebounce( setSearch, 500 );

const { searchResults } = useSelect(
( select ) => {
const { getEntityRecords } = select( coreStore );

return {
searchResults: !! search
? getEntityRecords( 'taxonomy', CATEGORY_SLUG, {
...DEFAULT_QUERY,
search,
} )
: EMPTY_ARRAY,
};
},
[ search ]
);

const suggestions = useMemo( () => {
return ( searchResults ?? [] ).map( ( term ) =>
unescapeString( term.name )
);
}, [ searchResults ] );
return ( Array.from( categoryMap.values() ) ?? [] )
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
.map( ( category ) => unescapeString( category.label ) )
.filter( ( category ) => {
if ( search !== '' ) {
return category
.toLowerCase()
.includes( search.toLowerCase() );
}
return true;
} )
.sort( ( a, b ) => a.localeCompare( b ) );
}, [ search, categoryMap ] );

function handleChange( termNames ) {
const uniqueTerms = termNames.reduce( ( terms, newTerm ) => {
Expand All @@ -64,17 +51,15 @@ export default function CategorySelector( { values, onChange } ) {
}

return (
<>
<FormTokenField
className="patterns-menu-items__convert-modal-categories"
value={ values }
suggestions={ suggestions }
onChange={ handleChange }
onInputChange={ debouncedSearch }
maxSuggestions={ MAX_TERMS_SUGGESTIONS }
label={ __( 'Categories' ) }
tokenizeOnBlur={ true }
/>
</>
<FormTokenField
className="patterns-menu-items__convert-modal-categories"
value={ categoryTerms }
suggestions={ suggestions }
onChange={ handleChange }
onInputChange={ debouncedSearch }
label={ __( 'Categories' ) }
tokenizeOnBlur={ true }
__experimentalExpandOnFocus={ true }
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
/>
);
}
53 changes: 49 additions & 4 deletions packages/patterns/src/components/create-pattern-modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import {
ToggleControl,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { useState } from '@wordpress/element';
import { useDispatch } from '@wordpress/data';
import { useState, useMemo } from '@wordpress/element';
import { useDispatch, useSelect } from '@wordpress/data';
import { store as noticesStore } from '@wordpress/notices';
import { store as coreStore } from '@wordpress/core-data';

Expand Down Expand Up @@ -42,6 +42,44 @@ export default function CreatePatternModal( {
const { saveEntityRecord, invalidateResolution } = useDispatch( coreStore );
const { createErrorNotice } = useDispatch( noticesStore );

const { corePatternCategories, userPatternCategories } = useSelect(
( select ) => {
const { getUserPatternCategories, getBlockPatternCategories } =
select( coreStore );

return {
corePatternCategories: getBlockPatternCategories(),
userPatternCategories: getUserPatternCategories(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it a bit weird that we have two functions + two REST APIs to retrieve pattern categories. Do we need to unify these at some point? I mean "core" and "user" can be query filters no? I guess things have evolved organically but maybe we should take a step back and assess whether the current APIs and storage we have is the right one for these things?

Copy link
Contributor Author

@glendaviesnz glendaviesnz Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this is a little weird, but we used the existing core taxonomy API endpoint for the user categories as a quicker path to get this into 6.4 with the shorter release cycle.

The plan is that now the user-created patterns and categories are in place to look at refactoring how these are retrieved and stored. As well as the category handling there is a bit of duplication between the site editor and block editor with how the patterns are handled so it would be good to see how we can abstract that away a bit. But we need to work out how best to do that while still keeping the block-editor uncoupled from the core data.

};
}
);

const categoryMap = useMemo( () => {
Copy link
Contributor Author

@glendaviesnz glendaviesnz Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

One question with merging is do we dedupe by label or slug and do we prioritise the core label/slug combinations over the user ones, or the other way around?

I think we should dedupe by label as below as that is what the user sees, so avoids confusion as currently, the likes of the core Headers/header label slug combination could lead to a user adding a Headers/headers combination and two Headers labels appearing. But, I don't have a strong opinion on whether the users categories should have priority over core or not.

Not sure if doing a ! uniqueCategories.has( category.label ) && check is any less/more performant than just doing an unnecessary uniqueCategories.set to avoid duplicates - with the small number of categories I am guessing 6 of one, half a dozen of the other.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a preference regarding deduping the labels or the slugs.

I don't think the micro-optimization matters here for performance until proven otherwise either. 👍

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 we should dedupe by label as below as that is what the user sees

This logic makes sense to me 👍

// Merge the user and core pattern categories and remove any duplicates.
const categories = [
...userPatternCategories,
...corePatternCategories,
].reduce( ( uniqueCategories, category ) => {
if (
! uniqueCategories.has( category.label ) &&
// There are two core categories with `Post` label so explictily remove the one with
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved
// the `query` slug to avoid any confusion.
category.name !== 'query'
) {
// We need to store the name separately as this is used as the slug in the
// taxonomy and may vary from the label.
uniqueCategories.set( category.label, {
label: category.label,
value: category.label,
name: category.name,
} );
}
return uniqueCategories;
}, new Map() );
Copy link
Member

Choose a reason for hiding this comment

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

Nit, non-blocking: Personally I would think a for loop could be easier to understand, but I can read this just fine too 👍 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Most people reading the code are probably still more use to simple for loops but I could be biased given that's my preference given too many use generic variable names in reduce e.g. accumulator. That's obviously not the case here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I quite like the slightly more self-contained nature of the .reduce for this sort of thing, but for the sake of quick readability let's switch it to a forEach - done.


return categories;
}, [ userPatternCategories, corePatternCategories ] );

async function onCreate( patternTitle, sync ) {
if ( ! title || isSaving ) {
return;
Expand Down Expand Up @@ -84,10 +122,16 @@ export default function CreatePatternModal( {
*/
async function findOrCreateTerm( term ) {
try {
// We need to match any existing term to the correct slug to prevent duplicates, eg.
// the core `Headers` category uses the singular `header` as the slug.
const existingTerm = categoryMap.get( term );
const termData = existingTerm
? { name: existingTerm.label, slug: existingTerm.name }
: { name: term };
const newTerm = await saveEntityRecord(
'taxonomy',
CATEGORY_SLUG,
{ name: term },
termData,
{ throwOnError: true }
);
invalidateResolution( 'getUserPatternCategories' );
Expand Down Expand Up @@ -126,8 +170,9 @@ export default function CreatePatternModal( {
className="patterns-create-modal__name-input"
/>
<CategorySelector
values={ categoryTerms }
categoryTerms={ categoryTerms }
onChange={ setCategoryTerms }
categoryMap={ categoryMap }
/>
<ToggleControl
label={ __( 'Synced' ) }
Expand Down
8 changes: 8 additions & 0 deletions packages/patterns/src/components/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@
.patterns-menu-items__convert-modal-categories {
max-width: 300px;
}
.components-form-token-field__suggestions-list {
position: absolute;
z-index: 1;
background-color: $white;
width: 295px;
min-width: initial;
border: 1px solid $gray-600;
}
}

.patterns-create-modal__name-input input[type="text"] {
Expand Down
Loading