Add "show template" to preview dropdown. #66514
Conversation
|
Size Change: +223 B (+0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 553af4d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11587116364
|
|
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. |
There was a problem hiding this comment.
Nice work getting this up so quickly, this is testing pretty well for me so far! Let's give it another round once the bug with the site editor is resolved (#66429 (comment)).
One tiny styling issue: the icon is a little closer to the edge of the DocumentBar than the keyboard shortcut icon. In the linked issue the padding / clearance area there is similar. Here's how it's looking for me locally:
Fixes #66429.
This change does most of it, but doesn't do the persistence mentioned in the issue. Shall we update the PR description to capture which parts of #66429 this implements?
Let's make sure persistence is the desired behaviour first! If so I can add it to this PR. |
|
I think this'll need a rebase now that #66540 has landed. |
tellthemachines
force-pushed
the
add/template-preview-in-dropdown
branch
from
91432c5
to
f808b34
Compare
There was a problem hiding this comment.
Visually this is looking nice to me!
While testing, I noticed that it's showing for me in the template part and pattern editors, too, where we don't have a concept of template previewing, so I wonder if we need a different check than ! isTemplate?
I'm not sure what the right check would be. I see that the PostTemplatePanel has some complexity to its checks (around here). The PostSummary component (its parent) uses a simpler check, though.
That's the only issue I ran into, though. Since the scope is still a little undecided re: persistence, I'll add gutenberg-design as reviewers, too.
|
Works well!
I guess it needs a code review. |
|
Thank you Isabel for this PR! It makes it a lot easier to notice that one can also view the template associated with the current page or post. |
There was a problem hiding this comment.
Thanks for the PR! There are three things I noticed:
- The button is 36px. The device switching button sizes are also 36px, so we may not need to address this in this PR. If we want to match the "Preview in new tab" button, 32px may be better:
- The icon in the document bar is squashed by padding, so it seems to be
20pxinstead of24px.margin-leftmight be more appropriate thanpadding-left. Additionally, the entire document bar is a button to launch the command palette, but the icon blocks that, so applyingpointer-events:noneto the icon might be a good idea.
Actual:
Expected:
- Regarding the following point:
While testing, I noticed that it's showing for me in the template part and pattern editors, too, where we don't have a concept of template previewing, so I wonder if we need a different check than
! isTemplate?
My understanding is that template parts and patterns are resizable canvases, so the device preview button should be disabled. I have submitted #65970 to fix this issue.
Great point! Sounds like once that lands the check in this PR might be fine, then? We can re-test once it lands. |
Just re-tested with a Classic theme and the 2024-10-30.11.51.11.mp4I see there's a check here that we might be able to borrow from: |
The difference is due to show template using the regular
Oh good catch! Weird that in TT1 at least the post title disappears when show template is toggled on 🤔 |
|
Hmmm. I think we have a bug where the template Id isn't always available. I was testing with Empty Theme, which has a generic Single Entries template for posts and pages, and it wasn't showing up. Then I changed to a different template for pages and suddenly the Single Entries template was recognised. What happens if there's no template Id is what we see in TT1: the "template view" only renders the post content. I'm going to add checks for both |
|
Actually, maybe we don't need to check whether it's a block theme? Can't we have ad hoc custom templates on classic themes too? |
I'd probably go with re-using the existing check for now ( |
|
Hmm I think I won't use |
Great catch! Yeah the |
I've noticed that #66596 is already in progress on this, so it won't need to be addressed in this PR 👍 |
|
I wanted to surface a bit of feedback I just got from a friend, that potentially this "Show template" toggle could also live here, in the main ellipsis menu, the "View" section:
Not a blocker necessarily for this PR, if we are happy to try it here and potentially move it later. Any thoughts? |
Hmm, I'm not sure. I think it makes more sense in the preview dropdown, because it's together with Mobile/Tablet etc which are basically preview tools to check what the post will look like on the front end, or on different device types. Whereas in the ellipsis menu, we have tools that change the editor view, with the intent of making writing and editing easier. But yeah, let's try it as it is and see how we go! Relative to the persistence issue I asked about here, do we still want to pursue it? |
|
I look at it this way... What are longer term plans for the preview dropdown? Has their been any tinkering with thinking how the preview dropdown might look like further down the road? Is there any preliminary designs one should start thinking about? What other features might the preview dropdown hold? Perhaps the preview dropdown might be later on expanded to having broader features then we currently have. |
|
I'm personally drawn to the View section now that I think about it, but happy to try it in the preview dropdown, especially if we believe we can move it at a later time if it doesn't feel right there. As far as persistence, I think we can also wait and listen to feedback on this first iteration of prominence. I think it has value, and speaks also to the ellipsis view section, because "Distraction free" also has persistance. But we can maybe combine those as a separate task if it feels useful. Let me know what you think.
Lots of thoughts, including some in #28466. Recently it received plugin extensibility. Tricky area to touch too drastically though, while some of the larger top toolbar designs are being refined. But mockups are always welcome. |
|
I don't think this option fits perfectly into any of the existing UI areas. If we're going to include an icon in the document title bar, this one:
Couldn't that be the toggle? Or open a menu including the options: 
|
|
It could be that toggle, except I initially added that icon to visually differentiate, somewhere in the UI, the difference between the editor with show template toggled on vs. off. I.e. you'd only see the template icon, if you see the template icon, it's because "Show template" is on. |
|
My point was it can (potentially) do both. When the template is visible the button is We might even include other template actions (swap, create new, edit) in this menu down the road. Essentially reuse the existing menu, in a more prominent location. |
If our objective is discoverability, my hunch is that the preview dropdown might be a better place for it as it's an area that folks use to interact with how they're previewing their content. Whereas the settings menu you might only go to for adjusting settings and not think to look there?
The document bar itself is one big button, so I'm not too sure if we can or should add a toggle within it. Or to put it differently, the idea of placing a toggle there might involve going down a rabbit hole of the inner workings of the For now, it looks like we have three options being discussed on this PR:
From my perspective it's working quite nicely in the Preview dropdown, and this is a small PR that could be fairly easily reverted. Would it be worth trying this option and continuing to iterate if we find we'd like to move it around further down the track?
and:
Lots of good ideas here, my vote is to look into these things in the longer-term, and try to land something smaller / simpler in the interim. |
Yep, sounds good to me. |
|
I would lean towards going with the current approach (preview dropdown). To do so, can we address the following points?
|
|
I gave this a little testing and spotted an issue in the Site editor. If you’ve first entered a focused edit mode and then went back to the page then unchecked "Show template" then the Back button shows up unexpectedly. back-wut.mp4In that demo I first edited the template but this issue also happens if you edit a template part or a synced pattern before unchecking "Show template". |
This might be related to the AnimationPresence. The BlockIcon is conditionally rendered in the AnimationPresence, which may be causing double animations on the back button. Moving the BlockIcon outside seems to solve the problem: diff --git a/packages/editor/src/components/document-bar/index.js b/packages/editor/src/components/document-bar/index.js
index f5de10d811..e95c43301e 100644
--- a/packages/editor/src/components/document-bar/index.js
+++ b/packages/editor/src/components/document-bar/index.js
@@ -145,13 +145,13 @@ export default function DocumentBar( props ) {
{ __( 'Back' ) }
</MotionButton>
) }
- { ! isTemplate && isTemplatePreview && (
- <BlockIcon
- icon={ layout }
- className="editor-document-bar__icon-layout"
- />
- ) }
</AnimatePresence>
+ { ! isTemplate && isTemplatePreview && (
+ <BlockIcon
+ icon={ layout }
+ className="editor-document-bar__icon-layout"
+ />
+ ) }
{ isNotFound ? (
<Text>{ __( 'Document not found' ) }</Text>
) : (
~ |
|
Thanks for the discussion folks! So it sounds like there are two things left to address before this can land?
Were they the only remaining concerns? |
|
Thanks for reviewing and testing folks! I've addressed all the pending feedback now. |
|
Thanks for the work! |
Co-authored-by: tellthemachines <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: paaljoachim <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: stokesman <[email protected]>
priethor
added
the
General Interface
What?
Fixes #66429.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast