-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Template Parts: Add further details to template part panel #52476
Conversation
Size Change: +264 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work on these improvements @jameskoster 👍
They test well for me overall although I did encounter one minor issue. When a custom template part area is registered and a template part is assigned to that, the area detail appears but is blank.
You can test this by:
- Registering a custom template part area via the snippet below
- Creating a template part via the site editor and assigning it to your custom template part area
- Selecting that template part via the Patterns page
Snippet to register custom template part area
<?php
function my_default_wp_template_part_areas( $default_area_definitions ) {
return array_merge(
$default_area_definitions,
array(
array(
'area' => 'my-area',
'label' => __( 'My Area' ),
'icon' => 'layout',
'description' => 'A custom template part area',
),
array(
'area' => 'custom-area',
'label' => __( 'Custom Area' ),
'icon' => 'layout',
'description' => 'Another custom template part area',
),
)
);
}
add_filter( 'default_wp_template_part_areas', 'my_default_wp_template_part_areas' );
In such a case, we might need to retrieve all the template part areas from the editor store and find the one matching the custom area slug to use its label.
If you'd like, I can help update this PR later this week after I've caught up from some recent AFK?
I've also left some other inline comments for minor tweaks such as styling the "details" and "navigation" h2 elements the same, removing unnecessary JSX fragments etc.
packages/edit-site/src/components/sidebar-navigation-screen-pattern/use-pattern-details.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-pattern/use-pattern-details.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-pattern/use-pattern-details.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/sidebar-navigation-screen-pattern/use-pattern-details.js
Outdated
Show resolved
Hide resolved
Thank you for reviewing my hacky designer code :) I may need your help with the custom areas. I found No rush, this is not a high priority item. |
Flaky tests detected in e6a5063. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5675556189
|
Not a problem. I'll try and get this updated with my "hacky engineer's code" in between other tasks today or tomorrow 😁 |
a19487b
to
d7a1b4e
Compare
I've made the tweaks to handle custom template part areas in the details. My only question is whether we should add a fallback label of some description in case a custom area is registered, a template part is created for it, and then the custom area is later removed. In that case, no template part area in the store would match the template part's slug, so no label would be shown in the area detail. |
What actually happens in that scenario? My assumption would be that the template part becomes uncategorized, IE "General". Is that not the case? If not, perhaps a "None" or "–" fallback label as you suggested could be useful. |
Looks good! |
While updating this PR I found an issue where orphaned template parts were included under the General category's count but not shown in the Patterns list. I've put up a fix for that in #52961. Combining that PR with this should allow all angles to be tested more effectively however I don't think it is necessary.
The "None" or "-" options do seem better than a blank field. I wonder if there is a third option, could we use the template part's area slug instead? It might give the user more of a clue where it was assigned etc? A comparison can be seen below:
Any preference @jameskoster or @jasmussen? |
"Areas" have been confusing me so I'll defer to Jay on that. |
Would it be possible to strip hyphens and sentence case the slug, and perhaps append "(Removed)"? IE:
That way the user is able to interpret the original purpose of the template part, and understand that the original area no longer exists.
I think the term "area" makes more sense in the context of editing a template. In template part management flows "Type" might be better. Probably one to look into in a separate PR so that we capture other "area" instances like the template part creation modal. |
db1b7dd
to
e6a5063
Compare
Certainly is. I've made this change in e6a5063 If you are happy with the current iteration, I can approve this PR so it can be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken this for another spin and kicked off the failing test again.
LGTM 👍
@aaronrobertshaw @jameskoster can we ensure we change the title of the PR before merging so that it doesn't read "Try..."? It's weird to see these in the changelog for a release. Once we are confident in the work let's reword it. |
Fixes #52377
What?
Add more information to template part detail panels:
Why?
Template part detail panels can often be a little empty making them feel somewhat superfluous.
Displaying more information here gives the panel purpose, and helps the user better understand the template part they are viewing.
More examples
Testing Instructions
Browse to the detail panels for the following template parts and observe associated details:
Custom template part
Theme template part
Plugin template part