Add "show template" to preview dropdown. #66514
Conversation
|
Size Change: +205 B (+0.01%) Total Size: 1.81 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? |
What?
Fixes #66429.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast