-
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
Move the template mode edit and new links to a dedicated template panel #30900
Conversation
a5e31f9
to
3001688
Compare
Size Change: +130 B (0%) Total Size: 1.46 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.
Code wise this looks good. If no other changes design wise occur, I'm pre approving. Thanks!
Left a comment just to fix the tests.
@@ -9,6 +9,7 @@ import { | |||
trashAllPosts, | |||
openPreviewPage, | |||
openDocumentSettingsSidebar, | |||
openSidebarPanelWithTitle, |
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.
This is not an exported util.
My feeling is that the answer to this question should be informed by the design solution for the top bar concept. The visual design may not be identical, but the iA should probably match as closely as possible. Those designs are forthcoming, but due to the volume of information we might need to display it seems fairly safe to assume that these settings will be split in to their own sections some how. All that to say – yes I believe a dedicated "Template" section in the Inspector is likely warranted. |
@@ -42,6 +43,7 @@ describe( 'Post Editor Template mode', () => { | |||
|
|||
// Switch to template mode. | |||
await openDocumentSettingsSidebar(); | |||
await openSidebarPanelWithTitle( 'Post Attributes' ); |
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.
Panels should be sentence case, i.e. "Post attributes". But it looks from the code that you're just opening an existing panel, and not introducing this here. So I guess it's a separate thing.
I was there when we added it to the status panel, but I also think it's right to move it out from there to reduce the noise, and longer term move it to the top toolbar as discussed. So moving it here makes sense: But perhaps it should be a secondary button, not a link button. To Jay's point, there's a bit of design work we need to do here, so there are some incoming design changes to this. But in the mean time, just a secondary button style might be a good start.
Maybe. But if that section, for now, will only contain "New" and "Edit", to me it seems fine to keep it in this attributes panel no? If it grows in time we can move it, perhaps. Not a strong opinion. Thanks for the PR! |
I was thinking of moving the template selector there as well. the idea for me was just to give this a bit more visibility. It's more important than say "parent page or order" |
It will also contain the selector, no? My train of thought was that the attributes panel could include the template details, post title input (for when/if we remove that from the canvas) and the permalink input. But we already have a dedicated permalink panel so there's no sense duplicating it. Medium term it will probably move to the top bar along with the title input anyway. At that point the attributes panel only contains template information. Ergo, we can probably just rename it "Template". |
Short term, until there is an updated template selector, it makes sense to group the template information and the page attributes template selection together. But with this PR, I can not see which template is being used? This was previously shown next to the edit link. |
I've updated this PR to use a dedicated panel, I think this works way better than what we have currently (or the previous implementation of this PR). Also, to address @carolinan's feedback, I added the template title to the panel title when available. |
0a12e4c
to
c723c1d
Compare
Right now, we have two places where we can view and edit the template of a page/post. The historical "page attributes" panel and the new "template" row under "Post status". This PR consolidates under the "Page/Post attributes" panel.
There are on going explorations to add flows related to the templates to the top toolbar (title) but this should be added separately.
That said I do wonder whether it makes sense to extract this from the "page attributes" panel into its own dedicated panel "template" right after "Post status". Did you consider that @jasmussen @jameskoster