Disable device preview button in pattern/template part/navitation editor #65970
Conversation
t-hamano
added
[Type] Bug
|
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. |
|
Size Change: -25 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
| @@ -154,7 +161,7 @@ function Header( { | |||
|
|
|||
| <PreviewDropdown | |||
| forceIsAutosaveable={ forceIsDirty } | |||
| disabled={ isNestedEntity } | |||
There was a problem hiding this comment.
Out of curiosity, when was this ever true?
There was a problem hiding this comment.
I think it gets defined here:
So I imagine it was intended if you go to edit a template and then click to edit a template part, then the preview would be disabled while editing the template part.
Incidentally, I think that behaviour had the unintentional effect that if you go to edit a post and then click to edit the template, the preview dropdown would be disabled. Here's trunk:
2024-10-30.11.40.04.mp4
With this PR applied, though, the preview dropdown is available while editing the template, which feels like an improvement to me. In the main, I think this is good, because it means that while previewing the whole template, users can switch between the different screen sizes:
2024-10-30.11.41.39.mp4
One interesting wrinkle in the above case is that while editing the template for a post, the preview link becomes "View site" and will link off to preview the site itself instead of the current post. As the user has switched to template editing, I don't think is a problem as the preview is contextual to what the user is currently doing, but just thought I'd mention it in case it wasn't expected.
There was a problem hiding this comment.
you go to edit a template and then click to edit a template part, then the preview would be disabled while editing the template part
This feels like a very roundabout way of addressing the issue, and more failure-prone than checking which entity is being edited. I wonder why that choice was made.
There was a problem hiding this comment.
This feels like a very roundabout way of addressing the issue, and more failure-prone than checking which entity is being edited. I wonder why that choice was made.
Yeah, I'm not sure. Being explicit about the entity feels safer and easier to read to me, too.
There was a problem hiding this comment.
Nice work here @t-hamano, overall I like the idea of tying this to the particular entities where we don't want previews rather than the presence of .onNavigateToPreviousEntityRecord, it feels more explicit to me 👍
I noticed that this also seems to fix a bug (I think it's a bug) where the preview options weren't available while editing a post's template. Just added a comment with what I'm seeing, but otherwise this is testing well for me so far!
| @@ -154,7 +161,7 @@ function Header( { | |||
|
|
|||
| <PreviewDropdown | |||
| forceIsAutosaveable={ forceIsDirty } | |||
| disabled={ isNestedEntity } | |||
There was a problem hiding this comment.
I think it gets defined here:
So I imagine it was intended if you go to edit a template and then click to edit a template part, then the preview would be disabled while editing the template part.
Incidentally, I think that behaviour had the unintentional effect that if you go to edit a post and then click to edit the template, the preview dropdown would be disabled. Here's trunk:
2024-10-30.11.40.04.mp4
With this PR applied, though, the preview dropdown is available while editing the template, which feels like an improvement to me. In the main, I think this is good, because it means that while previewing the whole template, users can switch between the different screen sizes:
2024-10-30.11.41.39.mp4
One interesting wrinkle in the above case is that while editing the template for a post, the preview link becomes "View site" and will link off to preview the site itself instead of the current post. As the user has switched to template editing, I don't think is a problem as the preview is contextual to what the user is currently doing, but just thought I'd mention it in case it wasn't expected.
|
Thanks for the review!
Thank you, I overlooked this point 😅
I'm struggling with how to address this issue in this PR. I can think of three options:
For now, I think option 1 is the best, but what do you think? |
|
@t-hamano I think we should ship this as is. "View site" is technically correct because what happens when you click the button is you go to the homepage. This is consistent with the template editing experience in the site editor. Arguably, it would be a nicer experience if that button took us to a page using the exact template being edited, but I can see how that might be non-trivial to implement, so for now I think do nothing is our best approach 😄 |
|
Thanks for laying out those options @t-hamano!
Me too. It took me by a little bit of surprise while editing the template, but not hugely so. And as soon as someone goes back to editing the post itself instead of the template, they get the correct view link. I'd lean toward landing this as-is, and we can always look into follow-ups if folks wind up finding it confusing. |
|
Thanks for the review! So let's merge this PR and move forward with #66514. |
…rdPress#65970) Co-authored-by: t-hamano <[email protected]> Co-authored-by: tellthemachines <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: draganescu <[email protected]>
Fixes #65931
What?
This PR disables the device preview button in the site editor, i.e. in resizable editors, where the device preview button should not be used.
Why?
Currently, whether to disable the device preview button is determined by whether the entity is nested, i.e.,
getEditorSettings().onNavigateToPreviousEntityRecord.This condition works in the post editor, but in the site editor, patterns, template parts, and navigation editors are not nested, so the device preview button is available unintentionally.
Resizable Editor and Device Preview currently cannot work together, so we should always disable the device preview button in a resizable Editor.
How?
This PR checks the post type, not whether the entity is nested.
Testing Instructions
Please check the entities below. The ✅ mark indicates the entities whose device preview button will be disabled by this PR.