Remove template-only mode from editor and edit-post packages
#57700
Conversation
|
Size Change: -2.66 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
There is still a bit of work/testing to do on this so not ready for review yet, eg. the e2e test failures look valid, probably related to the lack of a back link in the site editor now when navigating from page, so will look into those tomorrow. |
| if ( isEditingTemplate ) { | ||
| setRenderingMode( getEditorSettings().defaultRenderingMode ); | ||
| return; | ||
| } | ||
| if ( goBack ) { | ||
| goBack(); | ||
| } |
There was a problem hiding this comment.
This seems like the one place where we need a goBack method in the site editor's usePostLinkProps, and only when you have navigated from a page to edit its template. Still trying to figure out how best to handle this 🤔
There was a problem hiding this comment.
I wonder if the issue might be the code deleted on line 82-85, which seems important and maybe shouldn't be deleted. The defaultRenderingMode is one of template-locked or all, so not related to template-only.
Also noticing that currently when you edit a page, all blocks seems to be editable, compared to in trunk where only the post content is editable.
There was a problem hiding this comment.
I wonder if the issue might be the code deleted on line 82-85, which seems important and maybe shouldn't be deleted.
Because we are loading a new entity into the editor now on going back the default rendering mode will be set on the new editor load so I don't think we also need to do it here.
Also noticing that currently when you edit a page, all blocks seems to be editable, compared to in trunk where only the post content is editable.
This was caused by the way I was setting the default rendering mode - should be fixed now.
There was a problem hiding this comment.
Yes, the goBack method shouldn't passed to the site editor provider unless we actually went into focus mode, so the site editor need to be a second history of focused entities (in addition to the url history) kind of similar to the post editor. And navigating in the site editor outside these methods should reset the focused entities history.
There was a problem hiding this comment.
This was caused by the way I was setting the default rendering mode - should be fixed now.
Hmm, still doesn't seem to be right, the page content doesn't show 🤔
There was a problem hiding this comment.
for me goBack just means isFocusMode which translates to:
- Having a callback to go back to the previous focus entity or parent entity.
- Showing a padding around the canvas.
So we need in two places.
The other thing is, I'd like to arrive to a point in the next months where we could render the whole editor using <Editor post={post} settings={settings) /> to render the whole editor. You won't use the editor components one by one, which means at some point we need to address these specific components passed to specific components (probably become settings in most cases if really needed or just go away). That said, adding props to these components is fine temporarily (especially if these components are private APIs)
There was a problem hiding this comment.
Adding a goBack option in the site editor is a little more complex because it has a router and the back button works. At a quick glance it looks like we would need to add something like a popstate eventlistener and try and work out if we are coming back from a focus mode page in order to clear the goBack setting.
There may be an easier way, but I would like to suggest we keep it simple for this PR and just use the existing focusMode param that is used already in the site editor to get the back link (using the router back method) and the editor padding. This change does this and it seems to work, and we can always revisit this in a follow up if we do want to tie it in closer with the router.
There was a problem hiding this comment.
While it's a follow-up to a follow-up, making it another follow-up seems ok to me. This PR is getting quite big and comment-y, so breaking it out into a separate PR would make it easier to review.
There was a problem hiding this comment.
yeh, isFocusMode has been in the code base for at least 6 months, and this PR is intended to remove the recently added template-only mode before 6.5, so I think it makes sense rather than doing a follow-up directly after this to instead look at removing isFocusedMode as a separate task post 6.5, at which point we will have more time to plan and test more complex routing changes.
packages/editor/src/components/post-template/create-new-template-modal.js
Show resolved
Hide resolved
packages/edit-site/src/hooks/commands/use-edit-mode-commands.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/header/mode-switcher/index.js
Outdated
Show resolved
Hide resolved
| @@ -102,6 +105,8 @@ function Editor( { | |||
| ); | |||
|
|
|||
| const { updatePreferredStyleVariations } = useDispatch( editPostStore ); | |||
| const defaultRenderingMode = | |||
There was a problem hiding this comment.
I wonder if there's a way to merge "all" and "post-only" mode, what's the remaining difference between these two modes?
There was a problem hiding this comment.
post-only is still quite different to all, eg. with post-only the theme layout width, etc. are applied to the full post, page title displays larger, etc:
post-only:
all:
We may be able to refactor this somehow to get rid of the need for these modes, but I think we should limit this PR to template-only and revisit the other modes post 6.5.
packages/edit-site/src/components/sidebar-edit-mode/settings-header/index.js
Outdated
Show resolved
Hide resolved
|
Thanks for the feedback @youknowriad, I am AFK for a week, will follow up on your comments when I am back. |
| const { createSuccessNotice } = useDispatch( noticesStore ); | ||
| const { setRenderingMode } = useDispatch( editorStore ); | ||
| const editTemplate = getPostLinkProps | ||
| ? getPostLinkProps( { |
There was a problem hiding this comment.
I really don't like the name of the setting. I understand that it's needed because of the "href" but how important is it to have both the href and the onClick, onSelectEntityRecord would have been way better (or something like that).
There was a problem hiding this comment.
I agree. The naming was ok in the context of editing the synced patterns, but it hasn't extended well to this wider use case. As well as the setting naming, the onClick property is now also being used in places that don't involve a click.
I opened a PR here with some suggested renaming options, initially getEntityLoader, but @talldan thought the use of entity might be confused with the core-data apis.
That PR currently has the setting named as getPostNavigation, but the use of post is also not the best as we are using it to load templates and patterns. What about getEntityNavigation with properties of goTo and linkTo ?
There was a problem hiding this comment.
What I don't like is the "get" prefix, I prefer a callback directly "on" something, so my question is can't we get rid of the href and just leave the onClick?
There was a problem hiding this comment.
I actually think href is more important than onClick in terms of universal routing. In a future context, we might be able to just use regular <a> tags and href attributes for the post editor or shared packages (core-commands). In the site editor, we can attach an event listener globally that listens to all the bubbled events with the <a> links. Then, we can intercept the click and use history.push() under the hood to achieve seamless client-side routing.
href provides more context in this case, and it's accessible from the browsers' viewpoint as well (the bottom-left address when you hover over it and open-in-new-tab functionality, for instance.)
There was a problem hiding this comment.
I actually think href is more important than onClick in terms of universal routing.
I agree when it comes to links. The problem is that this is an API of the "block-editor" package and these actions are not always links. If I'm not wrong, there's no href in the post editor for instance, so it's highly dependent on the context. We're going to add an API that is not suited for all use-cases and forward compatibility might come and bite us later.
There was a problem hiding this comment.
The href was initially working in the post editor when we were updating the url params, but that caused other issues, so href has been broken in the post editor since we stopped updating the url. So, I have removed the href and renamed the setting to your suggestion @youknowriad of onSelectEntityRecord. There is a PR for this here.
I haven't extensively tested this yet, I will wait and see if you are happy with this new naming approach first - so don't spend too much time testing/reviewing, if you can let me know if this is along the lines of what you are thinking I will then give it a proper test and move it from draft.
packages/editor/src/components/post-template/create-new-template-modal.js
Outdated
Show resolved
Hide resolved
|
So If I'm not wrong, now the only missing thing is that the site editor is using a different "back" button than the post editor. I think we should follow-up fixing that but I also think that this PR is a great improvement as it is. Thanks for the work here :) |
| @@ -15,6 +15,10 @@ export function usePostLinkProps() { | |||
| const history = useHistory(); | |||
|
|
|||
| return ( params, state ) => { | |||
| return getPostLinkProps( history, params, state ); | |||
| return getPostLinkProps( | |||
There was a problem hiding this comment.
I believe getPostLinkProps is probably a function that shouldn't come from link.js file (which in my mind should't have "post" in it), maybe it's the name of the function that is wrong.
There was a problem hiding this comment.
Yeh, I think this ties in with the naming issue mentioned here. Given you approved this one I will assume you are happy to fix that in a follow-up rename PR. What did you think of getEntityNavigation ?
There was a problem hiding this comment.
I think it should move to the current file rather than stay in link basically, regardless of its name. That said, that's very very minor.
glendaviesnz
added
the
Backport to Gutenberg RC
…zy-hydration * origin/trunk: (47 commits) Interactivity API: Break up long hydration task in interactivity init (#58227) Add supports.interactivity to Query block (#58316) Font Library: Make notices more consistent (#58180) Fix Global styles text settings bleeding into placeholder component (#58303) Fix the position and size of the Options menu, (#57515) DataViews: Fix safari grid row height issue (#58302) Try a fix (#58282) Navigation Submenu Block: Make block name affect list view (#58296) Apply custom scroll style to fixed header block toolbar (#57444) Add support for transform and letter spacing controls in Global Styles > Typography > Elements (#58142) DataViews: Fix table view cell wrapper and BlockPreviews (#58062) Workflows: Add 'Technical Prototype' to the type-related labels list (#58163) Block Editor: Optimize the 'useBlockDisplayTitle' hook (#58250) Remove noahtallen from .wp-env codeowners (#58283) Global styles revisions: fix is-selected rules from affecting other areas of the editor (#58228) Try: Disable text selection for post content placeholder block. (#58169) Remove `template-only` mode from editor and edit-post packages (#57700) Refactored download/upload logic to support font faces with multiple src assets (#58216) Components: Expand theming support in COLORS (#58097) Implementing new UX for invoking rich text Link UI (#57986) ...
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
|
I just cherry-picked this PR to the release/17.6 branch to get it included in the next release: 6c7c352 |
cbravobernal
removed
the
Backport to Gutenberg RC
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
What?
Removes the
template-onlyeditor rendering mode.Why?
This mode was only added as a temporary measure to help with some of the work around integrating the site and post editors. With the work that was done in 57036 this can now be removed as it is now possible to inject a new post into the editor provider, so the current post can be replaced with the template.
How?
Uses the new
getPostLinkPropseditor setting to inject the template id and post type into the editor provider.Testing Instructions
Template - Edit Templateoption in the right post panelTemplate - Create new templateoption in right post panel and check it worksTemplate - Edit templateoption again and this time make sure theGo backoption in the success snackbar worksGo backbutton will not appear in the notification, this is because the routing works differently in the site editor, and the top back link is more prominant