Turn Template settings panel into a Template popover #41925
Conversation
noisysocks
added
[Type] Enhancement
noisysocks
changed the title
noisysocks
changed the title
|
Size Change: +963 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
packages/edit-post/src/components/sidebar/post-template/create-modal.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/post-template/create-modal.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/post-template/hook.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/post-template/hook.js
Outdated
Show resolved
Hide resolved
| { isPostsPage ? ( | ||
| <Notice | ||
| className="edit-post-post-template__notice" | ||
| status="warning" | ||
| isDismissible={ false } | ||
| > | ||
| { __( 'The posts page template cannot be changed.' ) } | ||
| </Notice> |
There was a problem hiding this comment.
This doesn't work because of the missing backport (#38607 (comment)), but you can use the following snippet for testing.
add_action( 'init', function() {
register_setting(
'reading',
'page_for_posts',
array(
'show_in_rest' => true,
'type' => 'number',
'description' => __( 'The ID of the page that should display the latest posts', 'gutenberg' ),
)
);
} );There was a problem hiding this comment.
Thanks! I was wondering what this was.
packages/edit-post/src/components/sidebar/post-template/hook.js
Outdated
Show resolved
Hide resolved
|
This change looks excellent. Fantastic work, @noisysocks! I left a few suggestions, happy to do more testing later today. |
packages/edit-post/src/components/sidebar/post-template/form.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/post-template/hook.js
Outdated
Show resolved
Hide resolved
| const canUserCreateTemplate = useSelect( | ||
| ( select ) => select( coreStore ).canUser( 'create', 'templates' ), | ||
| [] | ||
| ); |
There was a problem hiding this comment.
Might be overkill for this, but there's a useResourcePermissions hook that makes this easier - #38785
That also tells you if the entity can be edited when an id is provided. I notice you're using the 'create' permission for that at the moment. I don't know if there's any difference.
There was a problem hiding this comment.
Good tip about useResourcePermissions. I think it's overkill here while we're only checking create.
I didn't want to change any of the PostTemplatePanel logic as part of this PR. We only check create in trunk so that's what I'm doing here. I'm not sure if we should check edit. Probably a good idea to investigate that separately.
| position="bottom left" | ||
| contentClassName="edit-post-post-template__dialog" | ||
| renderToggle={ ( { isOpen, onToggle } ) => ( | ||
| <Button |
There was a problem hiding this comment.
Should it have aria-haspopup? I'm not sure what the right value would be, possibly 'dialog'. The trouble with that is the popover itself doesn't have that role. 🤔
Might be good to get some accessibility input on this.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-haspopup
There was a problem hiding this comment.
I'm not sure! 😅 If someone wants to weigh in and tell me what to do then I'm all ears.
All of the other popovers in the sidebar (post status, post date) currently work this way so I don't think it's a blocker. I'll be making more substantial changes to the panel as part of #39082 so perhaps can do some thorough accessibility testing then. One thing I noticed while working on this PR is that the labels aren't great. For example the post status button's label is Public which would sound strange out of context.
packages/edit-post/src/components/sidebar/post-template/style.scss
Outdated
Show resolved
Hide resolved
| .components-modal__header { | ||
| border-bottom: none; | ||
| } | ||
|
|
||
| .components-modal__content::before { | ||
| margin-bottom: $grid-unit-05; | ||
| } |
There was a problem hiding this comment.
Feels like this should be a prop on the Modal component to make it display this borderless variation. It'd be good to avoid overriding the component's style from outside.
It's brittle, because it can break when a component is refactored.
There was a problem hiding this comment.
Yeah totally agree. I'll do this separately as there's a few different parts of the codebase that we do this (welcome guide, list rename modal, start page options) and it warrants proper review.
packages/edit-post/src/components/sidebar/post-template/style.scss
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/post-template/index.js
Outdated
Show resolved
Hide resolved
|
Thanks for the reviews. I refactored the code a little bit so that we're not performing unnecessary data fetching. I'll work on addressing the components / styling feedback tomorrow. |
|
This looks fine! I have just two comments (maybe out of the scope for this PR):
Regarding the icon we use to add templates, I've created a proposal for a new one here:
@WordPress/gutenberg-design |
Good flag, I've made the text truncate.
I've fixed a bug which was making this pop-in slower than it should be, so this looks better now. I can't completely remove the pop-in. There seems to be a bug with how caching works in We could show an empty row or a spinner while we're waiting for the cached data but then this would look weird for users that do not have the necessary permissions to change the template.
Oh yep, thanks! Switched to that new icon. |
noisysocks
requested review from
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
OK think I've responded to everything and that this is ready for re-review! |
There was a problem hiding this comment.
This tests well for me. Thanks for fantastic work, @noisysocks 🦸
I left a small nitpick comment, but feel free to ignore it.
@javierarce, the "page for posts" notice might not fit well in your design, but happy to polish this as a follow-up.
Screenshot of notice
packages/edit-post/src/components/sidebar/post-template/create-modal.js
Outdated
Show resolved
Hide resolved
|
I think the changes here are generally an improvement. My one concern is that we may go in a completely different direction when we address #41258. Would it be harmful to repeatedly change the template UX? |
|
If there's a strong intention to go in that other direction and a timeframe for the change, I think it would probably make sense to reconsider this PR (but that's my personal opinion). Otherwise, I think the changes here (which are part of the changes required for #39082 and attempt to unify the way users manage the settings) make sense for the near future. |
|
Yeah it's a tough one... I don't think it's 100% clear which direction we'll head in just yet. Maybe since the Template panel is not currently a part of the Summary section we should consider it out of scope in the context of #39082? Especially since this PR doesn't address the underlying UX issues of that panel (making template selection a more visual experience). |
|
Ooh. If I had seen #41258 before starting on this then, yeah, it might have been worth holding off. But, I didn't! 😅 Since the work has been done and is ready to go, I think we may as well ship this as a small improvement on the path towards a more substantial improvement. I don't see a downside as this component is not a public API so it's quite easy to change or remove in the future. |
|
I'm going to go ahead and merge. We can continue to iterate as always. Thanks everyone for the really great feedback! |
|
Before: After: To me the result looks like a nice upgrade to what was before. This does feel like a part of the Summary section though, so that this PR should be linked in with the Summary section issue. Looking at the UX used in this PR for the Template to see if it is natural to bring it onward to other areas inside the Summary section. |
Do you mean #39082? It's linked. |
What?
Rewrites the Template panel in the sidebar to be a popover in the Summary (formerly Status & visibility) panel.
Why?
Part of #39082.
How?
TemplatePanelcomponent from@wordpress/edit-post.PostTemplatecomponent to@wordpress/edit-post.PostScheduleandPostVisibilitycomponents.TemplatePanel.I've moved most of the logic inside a hook so that there is no code duplication between the label (PostTemplate) and the form (PostTemplateForm).Testing Instructions
Screenshots or screencast
Kapture.2022-06-24.at.13.59.31.mp4