Allow adding new categories in Editor UI #123

mike-day · 2023-03-23T14:25:08Z

This PR allows the registration of custom pattern categories from within the editor UI.

Add a new category by simply typing in the categories dropdown.

Note: this PR should probably not be merged until #175, #177, and #180 are merged.

Registration is done in the pattern file itself, with the registration calls added just below the pattern header:

Using this method is almost akin to each pattern file being kind of like its own tiny plugin, with no patterns relying on each other for registration and each remaining their own source of truth.

…egistration calls to file

…k that register_block_pattern_category exists

mike-day · 2023-03-23T15:38:16Z

wp-modules/pattern-data-handlers/pattern-data-handlers.php

+		$formatted_registration_string = "if ( function_exists( 'register_block_pattern_category' ) ) {
+    " . implode( "\n    ", $custom_category_registrations ) . '
+}';


Same here — this is ugly but should allow linting to pass for the user.

Is there any time register_block_pattern_category() isn't defined?

It was introduced in WP 5.5.0.

Though maybe its file wasn't included when this code runs.

That's a good question!

My thinking was that we can't really know what version of WP an end-user of the theme might be using, but it's possible that we don't need to even worry about it.

Yeah, if it's only a question of the WP version, I think we can assume they have 5.5 or later.

PM requires 6.2 (though even then, it shouldn't require 6.2, as it's not released).

It seems like register_block_pattern was also introduced in 5.5.

Maybe it's worth investigating if the patterns directory is even checked for versions older than that. If they aren't, there is no reason to leave this check imo.

Upon investigating this further by testing with 5.4.12 — the registration block should not need to check that either WP_Block_Pattern_Categories_Registry or register_block_pattern_category() exists before calling them.

Both were introduced in WP 5.5, and previous versions of WP do not glob the patterns folder in a given theme.

wp-modules/editor/js/src/hooks/usePatternData.ts

kienstra

Hi @mike-day,
This approach looks good. It'd be nice to simplify the JS logic if possible, but this is a high-level review.

Pro

Single source of truth (PHP file). That's a huge plus.
This works even without PM active, like you showed.
By registering the custom category in the pattern's PHP file, a custom category only has to know about its own custom categories. If we registered all custom categories in a categories.php file, we'd have to tree shake unused categories after every save. It'd be much more complex.

Con

The JS logic here is really complex and untestable. Maybe it's too early to judge that, though 😄
Every pattern a user edits will have a 'Custom Categories' value in the header, even if they didn't add one. They might wonder why. Still, we could work on avoiding that.

mike-day · 2023-03-23T18:53:39Z

Great feedback, @kienstra!

As for Custom Categories always being added to the header — what would you think about it just being removed if there are no selections present? The registration statements are already removed, so it could make sense to remove it from the header as well since it's not something WordPress will recognize anyway.

kienstra · 2023-03-23T18:56:06Z

Great feedback, @kienstra!

As for Custom Categories always being added to the header — what would you think about it just being removed if there are selections present? The registration statements are already removed, so it could make sense to remove it from the header as well since it's not something WordPress will recognize anyway.

Yeah, that sounds good.

It's only a minor criticism. We could also refactor how we output the pattern file headers.

…g logic to new function

mike-day · 2023-03-23T20:28:11Z

@kienstra the Custom Categories header is rendered conditionally with 8771720.

kienstra · 2023-03-23T21:41:29Z

Hi @mike-day,
8771720 looks good. Maybe later we can consider doing something similar for other optional header values, but that's another discussion.

kienstra · 2023-03-23T21:43:42Z

Are you ready for feedback on whether this should check whether a category is already registered, and conditionally register it? (Not implementation details, but the idea of whether it should have a conditional.)

Don't want to nitpick if it's too early.

mike-day · 2023-03-24T03:47:50Z

Are you ready for feedback on whether this should check whether a category is already registered, and conditionally register it? (Not implementation details, but the idea of whether it should have a conditional.)

Don't want to nitpick if it's too early.

It might be a bit early! I just tried building from this PR to use a utility file instead of inserting registration calls in the pattern file with #124.

We also might want to try Mike's idea as an alternative as well.

My thought is that we can try a few proof-of-concept methods like this to gauge if it's worth pursuing custom category registration at all — it's going to add complexity to the app no matter what.

kienstra · 2023-03-24T15:54:53Z

It might be a bit early! I just tried building from this PR to use a utility file instead of inserting registration calls in the pattern file with #124.

Sounds good!

…e core store to get registered categories instead of hydration

…ndlers

mike-day · 2023-03-24T17:22:34Z

@kienstra I made a few small changes (including using a prepend with the new category name instead of meta — it wasn't working consistently that way).

Let's get into the conversation around checking against the category registry!

We don't really need to check against the registry. Core will only register a given pattern category once, regardless of how many times that call is made. We could get rid of some boilerplate if we didn't make that check.

On the other hand, it's hard to know whether that behavior in core is a feature or a bug.

Otherwise, it does feel "right" to me to conditionally register, but I can't really articulate why.

kienstra · 2023-03-24T19:05:26Z

That's a good point. Let me think about this.

mike-day · 2023-03-25T22:26:34Z

Looking at the register() method in the block pattern category registry, it seems that there is no logic to check if an incoming category is already registered.

Instead, if the name of the new category matches an existing category, the existing category is simply overwritten:

$category = array_merge(
	array( 'name' => $category_name ),
	$category_properties
);

$this->registered_categories[ $category_name ] = $category;

So, while it initially appears that register_block_pattern_category() call be called with the same $category_name any number of times and only register the category once, the new category is actually being registered/overwritten repeatedly.

…concise

kienstra

Hi @mike-day,
The tests look really good. Very good coverage, and good work extracting into pure functions.

kienstra · 2023-05-22T19:41:50Z

wp-modules/editor/js/src/components/SidebarPanels/CategoriesPanel.tsx

-							( matchedCategory ) =>
-								matchedCategory.value === category
-						)
+					value={ getSelectedOptions(


Really nice extraction to a pure function

wp-modules/pattern-data-handlers/pattern-data-handlers.php

kienstra · 2023-05-22T19:55:46Z

wp-modules/pattern-data-handlers/tests/PatternDataHandlersTest.php

+	/**
+	 * Tests maybe_add_custom_category_header.
+	 */
+	public function test_maybe_add_custom_category_header() {


Nice test. It's simple, but covers the 2 important cases: where there is a header, and there isn't.

kienstra · 2023-05-22T20:12:09Z

wp-modules/pattern-data-handlers/tests/PatternDataHandlersTest.php

+			$this->normalize( create_formatted_category_registrations( $custom_categories ) ),
+		);
+
+		$this->assertEmpty( create_formatted_category_registrations( [] ) );


Another nice test. It's simple, and covers the main cases.

kienstra · 2023-05-22T20:13:27Z

wp-modules/pattern-data-handlers/pattern-data-handlers.php

+
+	return $custom_category_registrations;
+}
+


In a future PR, let's split this file into another. It's getting long.

Agreed — we could clean up this file dramatically by splitting things up.

kienstra · 2023-05-22T20:43:51Z

wp-modules/editor/js/src/utils/test/getCustomCategories.ts

+			[ 'Second Category', 'Third Category' ],
+		],
+	] )(
+		'should get the custom categories created by Pattern Manager',


Nice test coverage, from [], to matched categories that don't have pm_custom, to matched categories that do have pm_custom.

kienstra

Hi @mike-day,
The tests look really good. Very good coverage, and good work extracting into pure functions.

…into try/add-new-categories-in-editor-ui

… code review)

…into try/add-new-categories-in-editor-ui

johnstonphilip · 2023-06-01T20:08:26Z

This is really impressive @mike-day!

I've found a bug which we might want to fix. The desired intention is that custom categories registered in a theme do not get written to a pattern file. But they do in this scenario:

Add a custom category to a pattern using the UI
Now, register that category in your theme.
Re-save the original pattern, notice the custom category registration does not get stripped out of the file

I think to fix this, we'll need to add some logic to the get_pattern_by_path to double check for custom category existence, and strip them out there if so.

johnstonphilip · 2023-06-01T20:42:32Z

Upon digging into this, it's a bit harder than I first thought. Because pattern files are actually being included in format_pattern_data, it means all pattern categories are registered before we save, so we don't have a reliable way of knowing if a pattern category was registered from the theme's functions file, or the pattern file itself.

johnstonphilip

Since trying to prevent category registration has only cosmetic benefits right now, I think we can merge this as-is.

…into try/add-new-categories-in-editor-ui

kienstra · 2023-06-15T22:03:37Z

Sorry, I accidentally merged #199. Reverting now.

This reverts commit 48ae649.

This reverts commit f5fcdcf.

kienstra · 2023-06-15T22:04:57Z

Sorry, I won't commit to this PR anymore. Just wanted to undo my accidental merge of #199.

…IllegalChars

…-input Validate new categories on input

…into try/add-new-categories-in-editor-ui

mike-day added 6 commits March 23, 2023 09:15

Register new meta; hydrate editor with patternCategories

a59ecdc

Adjust types for new customCategories, global

867881f

Update usePatternData to use global, create combinedCategories

477d3f3

Add customCategories to pattern-data-handlers, parse in header, add r…

a1e7af1

…egistration calls to file

Use customCategories in CategoriesPanel

ebaa2c6

Move registration creation to it's own function; add conditional chec…

b562add

…k that register_block_pattern_category exists

mike-day commented Mar 23, 2023

View reviewed changes

kienstra reviewed Mar 23, 2023

View reviewed changes

wp-modules/editor/js/src/hooks/usePatternData.ts Outdated Show resolved Hide resolved

kienstra approved these changes Mar 23, 2023

View reviewed changes

mike-day added 3 commits March 23, 2023 15:17

Add registry check before registering a category; move string buildin…

65b77cb

…g logic to new function

Conditionally add/remove Custom Category header

8771720

Add missing doc for maybe_add_custom_category_header

6b47385

mike-day mentioned this pull request Mar 24, 2023

[DO NOT MERGE] Use category registration utility file in the patterns dir #124

Closed

mike-day changed the title ~~Allow adding new categories in Editor UI~~ [DO NOT MERGE] Allow adding new categories in Editor UI Mar 24, 2023

mike-day added 3 commits March 24, 2023 12:03

Use prepended string to find custom categories instead of pm_meta; us…

c9f0ad3

…e core store to get registered categories instead of hydration

Try to make the registration string easier to read in pattern-data-ha…

2dce936

…ndlers

Linting

731932f

mike-day added 3 commits March 27, 2023 09:39

Remove unneeded $registered_categories, make registration block more …

88f460d

…concise

Try to resolve CCI PHP linting error by removing inline function

109d9ad

Remove unused function maybe_display_formatted_category_registrations

601b302

kienstra approved these changes May 22, 2023

View reviewed changes

mike-day added 6 commits May 22, 2023 16:41

Merge branch 'main' of https://github.com/studiopress/pattern-manager …

82beb35

…into try/add-new-categories-in-editor-ui

Create addNewCategory util

bdcce92

Use the new util in usePatternData

6d5f2f5

Condense the category header, registration functions (suggestion from…

64216ad

… code review)

Merge branch 'main' of https://github.com/studiopress/pattern-manager …

f2f180c

…into try/add-new-categories-in-editor-ui

Use toKebabCase instead of convertToSlug

4a57894

mike-day marked this pull request as ready for review June 1, 2023 19:09

johnstonphilip approved these changes Jun 14, 2023

View reviewed changes

mike-day and others added 6 commits June 14, 2023 11:17

Merge branch 'main' of https://github.com/studiopress/pattern-manager …

9dc9099

…into try/add-new-categories-in-editor-ui

Create validateInput utility

45d4f26

Use the new util; style the input for illegal chars

ca0e239

Format the creation text to also strip illegal chars

6f9e726

See if slashing pattern categories helps

f5fcdcf

Fix linting errors

48ae649

kienstra added 2 commits June 15, 2023 16:03

Revert "Fix linting errors"

99a2bde

This reverts commit 48ae649.

Revert "See if slashing pattern categories helps"

6f707e1

This reverts commit f5fcdcf.

mike-day and others added 6 commits June 15, 2023 20:22

Use Ryan's suggestion and return a bool from util; update name to has…

0e133b2

…IllegalChars

Make sure categories array also gets the validated title

2dbe129

Fix a typo in text case decription

00bee10

Merge pull request #198 from studiopress/bugfix/validate-new-category…

bf3d3e8

…-input Validate new categories on input

Prevent the adding of an empty category name

fa190d8

Merge branch 'main' of https://github.com/studiopress/pattern-manager …

31908ec

…into try/add-new-categories-in-editor-ui

mike-day merged commit f2a7b26 into main Jun 16, 2023

mike-day deleted the try/add-new-categories-in-editor-ui branch June 16, 2023 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow adding new categories in Editor UI #123

Allow adding new categories in Editor UI #123

mike-day commented Mar 23, 2023 •

edited

Loading

mike-day Mar 23, 2023 •

edited

Loading

kienstra Mar 23, 2023 •

edited

Loading

mike-day Mar 23, 2023

kienstra Mar 23, 2023 •

edited

Loading

mike-day Mar 23, 2023

mike-day Mar 27, 2023

kienstra Mar 27, 2023

kienstra left a comment •

edited

Loading

mike-day commented Mar 23, 2023 •

edited

Loading

kienstra commented Mar 23, 2023

mike-day commented Mar 23, 2023

kienstra commented Mar 23, 2023 •

edited

Loading

kienstra commented Mar 23, 2023 •

edited

Loading

mike-day commented Mar 24, 2023

kienstra commented Mar 24, 2023

mike-day commented Mar 24, 2023 •

edited

Loading

kienstra commented Mar 24, 2023

mike-day commented Mar 25, 2023 •

edited

Loading

kienstra left a comment

kienstra May 22, 2023

kienstra May 22, 2023

kienstra May 22, 2023

kienstra May 22, 2023

mike-day May 24, 2023

kienstra May 22, 2023

kienstra left a comment

johnstonphilip commented Jun 1, 2023 •

edited

Loading

johnstonphilip commented Jun 1, 2023 •

edited

Loading

johnstonphilip left a comment

kienstra commented Jun 15, 2023

kienstra commented Jun 15, 2023

Allow adding new categories in Editor UI #123

Allow adding new categories in Editor UI #123

Conversation

mike-day commented Mar 23, 2023 • edited Loading

mike-day Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

kienstra Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra left a comment • edited Loading

Choose a reason for hiding this comment

Pro

Con

mike-day commented Mar 23, 2023 • edited Loading

kienstra commented Mar 23, 2023

mike-day commented Mar 23, 2023

kienstra commented Mar 23, 2023 • edited Loading

kienstra commented Mar 23, 2023 • edited Loading

mike-day commented Mar 24, 2023

kienstra commented Mar 24, 2023

mike-day commented Mar 24, 2023 • edited Loading

kienstra commented Mar 24, 2023

mike-day commented Mar 25, 2023 • edited Loading

kienstra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra left a comment

Choose a reason for hiding this comment

johnstonphilip commented Jun 1, 2023 • edited Loading

johnstonphilip commented Jun 1, 2023 • edited Loading

johnstonphilip left a comment

Choose a reason for hiding this comment

kienstra commented Jun 15, 2023

kienstra commented Jun 15, 2023

mike-day commented Mar 23, 2023 •

edited

Loading

mike-day Mar 23, 2023 •

edited

Loading

kienstra Mar 23, 2023 •

edited

Loading

kienstra Mar 23, 2023 •

edited

Loading

kienstra left a comment •

edited

Loading

mike-day commented Mar 23, 2023 •

edited

Loading

kienstra commented Mar 23, 2023 •

edited

Loading

kienstra commented Mar 23, 2023 •

edited

Loading

mike-day commented Mar 24, 2023 •

edited

Loading

mike-day commented Mar 25, 2023 •

edited

Loading

johnstonphilip commented Jun 1, 2023 •

edited

Loading

johnstonphilip commented Jun 1, 2023 •

edited

Loading