Site editor: consolidate constants#54484
Conversation
ramonjd
added
[Type] Code Quality
ramonjd
requested review from
SaxonF,
aaronrobertshaw,
andrewserong,
glendaviesnz,
kevin940726 and
tellthemachines
| export const USER_PATTERNS = 'wp_block'; | ||
| export const USER_PATTERN_CATEGORY = 'my-patterns'; |
There was a problem hiding this comment.
Was there a reason to split USER and DEFAULT even though they have the same value? If so I can re-split.
| 'pattern-directory/featured', | ||
| 'pattern-directory/theme', | ||
| ]; | ||
| export const PATTERN_SYNC_STATUSES = { |
There was a problem hiding this comment.
Renamed from "type" to "status" since I thought it might be confused with post types.
There was a problem hiding this comment.
Just for background the reason for type versus status was to avoid any confusion with the idea that a pattern was currently unsynced because the sync had not been run yet and full because the sync had been run and completed successfully, ie. sync status gives the idea that is something that changes as a sync process runs, whereas type indicates more of a static setting. But I don't have a strong opinion either way on this.
There was a problem hiding this comment.
Thanks for the context. Also happy to switch it back if that's the case. I was running under the assumption they were just labels for statuses, and didn't reflect the status of any entity.
|
Size Change: +154 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
update/consolidate-edit-site-constants
branch
from
0bf3bb0 to
e85b529
Compare
There was a problem hiding this comment.
I couldn't find any reference to ALL_PATTERNS_CATEGORY, so I think it's safe to just assign the default category value here.
There was a problem hiding this comment.
I think the last reference to it was refactored into the patterns package.
There was a problem hiding this comment.
Oh, thanks for flagging. I assume it's not such a huge deal there since we probably won't be exporting the site editor constants (?)
I guess I should rename for consistency.
There was a problem hiding this comment.
And SYNC_TYPES and the rest in the patterns package.
There was a problem hiding this comment.
Merging DEFAULT_CATEGORY and ALL_PATTERNS_CATEGORY into PATTERN_DEFAULT_CATEGORY makes sense to me
There was a problem hiding this comment.
Makes sense to me as well.
The only tweak I'd suggest is perhaps adding an explanatory inline comment to the pattern filtering where it might make it clearer why the default category is being used in the conditions. For example, that it's really revolving around whether all patterns are being shown as opposed to some specific default category.
There was a problem hiding this comment.
There was a problem hiding this comment.
Looks like we're using hardcoded strings in places where this might have been used previously. Should it just be removed?
There was a problem hiding this comment.
I'm not sure, I'll take your advice on this.
I see a lot of use of PATTERNS around, which PATTERN_THEME_TYPE replaces in this PR.
There was a problem hiding this comment.
I wonder if an object of PATTERN_TYPES would be clearer:
export const PATTERN_TYPES = {
theme: 'pattern',
user: 'wp_block'
}then used as PATTERN_TYPE.theme & PATTERN_TYPE.user. User is probably more descriptive of the source than post.
There was a problem hiding this comment.
then used as PATTERN_TYPE.theme & PATTERN_TYPE.user. User is probably more descriptive of the source than post.
If that's clearer to you folks, who have been working on this, then it sounds good to me. 😄
By "post" I was trying to allude to the fact that it's a custom post type (inheriting the post object).
Primarily hoisting pattern constants and removing duplicates.
ramonjd
force-pushed
the
update/consolidate-edit-site-constants
branch
from
e85b529 to
67d3e69
Compare
| import { store as noticesStore } from '@wordpress/notices'; | ||
|
|
||
| export const ALL_PATTERNS_CATEGORY = 'all-patterns'; | ||
| export const PATTERN_DEFAULT_CATEGORY = 'all-patterns'; |
There was a problem hiding this comment.
Just to make the nomenclature consistent with the site editor constants.
|
@ramonjd thanks for working on this, it certainly needed a tidy-up. I think we should move the pattern constants to the patterns package so we can then do the same tidy-up in the block editor. What do you think of this? Could be reverted if you think this is better in a follow up. I also went back to the sync type and added a PATTERN_TYPE object. |
This is a good idea! Thanks for implementing it. Since the main aim of this PR is to consolidate duplicates and centralize things, personally I'm not too fussed about how we do it. So it LGTM. |
|
Flaky tests detected in f54bf74. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6218204672
|
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
The consolidation of these constants makes sense to me, including moving those that can be to the patterns package.
I haven't noticed any regressions, so far the Site Editor is testing well.
✅ Patterns - Can create, rename, duplicate, delete, search, view & edit etc
✅ Template Parts - Can create, rename, duplicate, delete, search, edit, clear customizations etc
✅ Templates - Creation, editing, renaming, navigation etc continue to work for me
While the code changes LGTM, given the number of places tweaked, it might pay to get a second set of eyes involved on the testing.
After f54bf74, we could probably do with updating the PR description for posterity as well.
That was a dad joke.
Fixed this also 😉
aaronrobertshaw
added
the
[Feature] Patterns
| 'Editing the pattern will update it anywhere it is used.' | ||
| ) } | ||
| checked={ ! syncType } | ||
| checked={ syncType === PATTERN_SYNC_TYPES.full } |
There was a problem hiding this comment.
This is not the same as !syncType. IIRC, fully synced patterns have empty syncStatus for backward-compatible reason? I think this only matters when we create the pattern and if it handles 'fully' or '' in onCreate 🤔 . @glendaviesnz or @aaronrobertshaw might have some insights on this.
There was a problem hiding this comment.
Good catch, I missed it in my review 😥
We should maintain the same logic.
There was a problem hiding this comment.
correct, but the only place where it is actually set to undefined is in the action here, and that still works as expected as everything except syncType of unsynced is set there as undefined.
There was a problem hiding this comment.
Thanks for catching that. That was a bad rebase on my part probably. I'll fix it up.
There was a problem hiding this comment.
correct, but the only place where it is actually set to undefined is in the action here, and that still works as expected as everything except syncType of unsynced is set there as undefined.
Oh, sorry I just saw this. No need to revert?
There was a problem hiding this comment.
Yeah @glendaviesnz is correct. I don't think this introduces any unexpected behaviors. As a matter of fact, I think this is better than undefined as it's explicit and it matches the types in the actions. 👍
Added clarification comment to category filtering.
kevin940726
left a comment
kevin940726
left a comment
There was a problem hiding this comment.
I'll give you another thumbsup because you deserve it 😆
|
Thanks for helping out on this one, folks 🙏🏻 |
Follow up on #54484 to consolidate constants
Follow up on #54484 to consolidate constants
…54586) * Initial commit: Follow up on #54484 to consolidate constants * More using constants instead of hard-coded strings * Rename TEMPLATE_CUSTOM_SOURCE to TEMPLATE_ORIGINS * Update packages/edit-site/src/components/sidebar-navigation-screen-pattern/use-pattern-details.js Co-authored-by: Kai Hao <kevin830726@gmail.com> --------- Co-authored-by: Kai Hao <kevin830726@gmail.com>
|
It looks like I missed a regression in my testing sorry! A git bisect pointed to this commit while chasing down a bug where only user patterns show for the "All Patterns" category when first navigating to the patterns page. Screen.Recording.2023-09-22.at.7.26.40.pm.mp4A fix is up in #54721 and should land soon. |
|
Turns out there was another fallback category type that didn't use the original I've put up a quick PR to flip that back to its correct value. |
…54586) * Initial commit: Follow up on #54484 to consolidate constants * More using constants instead of hard-coded strings * Rename TEMPLATE_CUSTOM_SOURCE to TEMPLATE_ORIGINS * Update packages/edit-site/src/components/sidebar-navigation-screen-pattern/use-pattern-details.js Co-authored-by: Kai Hao <kevin830726@gmail.com> --------- Co-authored-by: Kai Hao <kevin830726@gmail.com>
5 participants
❗ Don't merge until the following PR is merged:
What?
Consolidating constants in the edit-site package.
Primarily hoisting pattern constants and removing duplicates.
I also removed duplicate constants.
That was a joke.
Move pattern-specifc constants to the patterns package and update its files to use them.
To keep this PR small, I haven't replaced all hard-coded values with CONSTANTS, but can do in a follow up once we've finalized the approach.
Why?
While working on unifying the template/pattern actions drop down component, I noticed that there were a few constants that we could use across the edit-site package.
Also, it would be good to centralize our references to post types.
Testing Instructions
The site editor in general should work, tests should pass.
In particular, create, rename, and generally manage patterns and template parts in the site editor. Path:
/wp-admin/site-editor.php?categoryType=wp_block&categoryId=my-patterns&path=%2Fpatterns