Patterns: Avoid mapping template parts objects to patterns #62927
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
youknowriad
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -14.6 kB (-0.83%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
ellatrix
reviewed
Jun 27, 2024
There was a problem hiding this comment.
I'm not sure what to test specifically. Do we have e2e test coverage for template parts/patterns?
I did get a warning react-jsx-runtime.development.js:87 Warning: Each child in a list should have a unique "key" prop. when loading the "all template parts" page.
ntsekouras
reviewed
Jun 28, 2024
ntsekouras
reviewed
Jun 28, 2024
ntsekouras
reviewed
Jun 28, 2024
oandregal
reviewed
Jun 28, 2024
oandregal
reviewed
Jun 28, 2024
oandregal
reviewed
Jun 28, 2024
| canvas: 'edit', | ||
| } ); | ||
| const title = decodeEntities( defaultGetTitle( item ) ); |
There was a problem hiding this comment.
Nice catch 👍
Before:
Gravacao.do.ecra.2024-06-28.as.13.34.54.mov
After:
Gravacao.do.ecra.2024-06-28.as.13.32.12.mov
oandregal
reviewed
Jun 28, 2024
oandregal
reviewed
Jun 28, 2024
oandregal
reviewed
Jun 28, 2024
| categoryId, | ||
| search | ||
| ); | ||
| return selectTemplateParts( select, categoryId, search ); |
There was a problem hiding this comment.
I've only found two uses of the usePatterns hook:
- One is in the page-patterns screen, also adapted in this PR.
- The other is in the sidebar, that doesn't need any adaption as it's only used to retrieve data from patterns but not from template parts post types (what is being modified in this PR).
There was a problem hiding this comment.
I presume that, back in the days, the usePatterns hook was created as a way to avoid if conditions in the page (rules of hooks) and also to provide a stable structure to consumers (sidebar, page) independently from where the data came from. It seems to me the consumers still do a lot of data wrangling depending on the type, so not sure how effective this has been for the second goal.
It'd be good to rename it to usePatternsOrParts of something similar for clarity at some point: not a requirement for this PR.
oandregal
reviewed
Jun 28, 2024
| theme && slug ? theme + '//' + slug : null; | ||
|
|
||
| const templatePartToPattern = ( templatePart ) => ( { | ||
| blocks: parse( templatePart.content.raw, { |
There was a problem hiding this comment.
Reviewed how this (or whether) is being used in the consumer code that uses parts (page-patterns). Everything has been migrated:
- blocks: adapted consumer code
- categories: unused
- description: no need to do anything
- isCustom: unused
- keywords: unused
- id: parts already have an id
- name: removed & adapted the consumer code
- title: removed & adapted the consumer code
- type: as it is
_links: as it is
There was a problem hiding this comment.
I see actions were also a consumer of this (through dataviews's item). I've only found one case that requires updating (title is not using decodedEntities https://github.com/WordPress/gutenberg/pull/62927/files#r1658485082 ) but the rest were already ported (isCustom, etc.).
oandregal
reviewed
Jun 28, 2024
There was a problem hiding this comment.
Code-wise, this is a good move. The usePatterns hook and the idea of providing a stable structure for the page-patterns across patterns & parts wasn't working: it leaked to actions & the page-patterns still had to do a lot of data wrangling depending on the post type.
I've done some testing: both the Patterns dataviews page & actions within the editor.
Issues:
- Parts: duplicate action doesn't work properly (it works in
trunk).- in dataviews: shows weird title (object, not the title string)
- in editor: it triggers an error (probably related)
- User patterns: the title for the duplicate action needs passing through
decodeEntities(creating a pattern whose name isandréshould help testing this: compare duplicate action vs rename action to see the difference)
Edit:
Duplicate (dataviews):
Duplicate (editor):
|
Thanks for the thorough testing @oandregal, we definitely need more e2e test. Probably one per action. I'm going to try to add some as I refactor and type the actions. |
|
We should also look at removing |
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.
Part of #55083
Related #59659
Unblocks #62913
What?
When working on a the dataviews extensibility actions. I noticed that some of the post type actions receive objects that are not actual post types, they are temporary objects that were created for the purpose of rendering template parts in the patterns page. We were mapping template parts to weird objects that resemble patterns.
For me this is not something we should be doing because:
How?
This PR just removes that mapping and tries to update all the places where that mapping was necessary. The code is a bit hard to follow, so there's a chance that I missed some places. Some help testing everything is needed.
Unrelated: I was not able to test the template parts actions menu in the editor because I was not able to open template parts in the editor (I'll investigate this separately)
Note I noticed that there's also mapping happening between registered patterns (theme patterns) and saved patterns (user patterns). I think for these as well, we should be removing the mapping.
Testing Instructions
1- Use the different filters, fields and actions in the template parts dataviews.