Fix custom template part theme meta. #26587
Merged
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
Addison-Stavlo
requested review from
mattwiebe,
vindl,
Copons,
david-szabo97,
jeyip and
noahtallen
Addison-Stavlo
requested review from
ajitbohra,
Soean and
talldan
as code owners
|
Size Change: +7 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
Addison-Stavlo
force-pushed
the
fix/custom-template-part-theme-name
branch
from
7ee34cf
to
c2182c6
Compare
Copons
reviewed
Oct 30, 2020
| <PanelGroup | ||
| key={ templatePart.id } | ||
| title={ templatePart.meta.theme || __( 'Custom' ) } | ||
| > |
There was a problem hiding this comment.
Change makes sense to me, but I've noticed that when searching, template parts aren't grouped by theme, but each part is in its own theme group:
| Normal List | Search Results |
|---|---|
 |
 |
(This is not related to this PR, but I just thought of mentioning it)
There was a problem hiding this comment.
Yeah, the search results group them individually as they are ordered a bit naively by the priorities:
- index match is found in the slug.
- index match is found in the theme.
I think a lot of this does need to be rethought in the future.
Copons
reviewed
Oct 30, 2020
| @@ -109,7 +109,8 @@ function TemplatePartsByTheme( { | |||
| return templatePartsByTheme.map( ( templatePartList ) => ( | |||
| <PanelGroup | |||
| key={ templatePartList[ 0 ].meta.theme } | |||
There was a problem hiding this comment.
I think we should add a fallback value here as well, to avoid an empty key. 🤔
There was a problem hiding this comment.
Possibly? I think React doesn't get mad if the key is the empty string, like it does when it is null/undefined. So in this case '' may still be a valid unique key for the group?
There was a problem hiding this comment.
Yeah I've noticed React didn't complain about it, so... LGTM then!
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.
Description
As discussed on #26394 (comment) - it makes more sense to create custom template parts with a blank theme name than 'Custom'. This will ensure consistency with template parts created from the wp-admin template-part menu, as well as with how templates themselves denote theme names.
This PR updates the custom template part creation to create with the theme name
''and updates the selection previews to show'Custom'when the theme name is empty.How has this been tested?
Screenshots
Types of changes
Checklist: