Editor: Move the template focus modes to the editor store #56472
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
youknowriad
commented
Nov 23, 2023
| @@ -133,6 +132,7 @@ export default function Editor( { listViewToggleElement, isLoading } ) { | |||
| : undefined, | |||
| editorMode: getEditorMode(), | |||
| canvasMode: getCanvasMode(), | |||
| renderingMode: getRenderingMode(), | |||
There was a problem hiding this comment.
We have so many modes: canvasMode, editorMode, renderingMode. To be honest, I don't like that and I'm not sure how to better name this. I'm opening to changing the name. Could be contentMode or focusMode or contentFocusMode or something. I'm not sure, but I know that focusMode already exists for something else.
There was a problem hiding this comment.
There are two modes that use "focus" as far as I know:
- "Spotlight mode" in preferences (
focusModevariable) - Focus mode is also used to describe how the editor behaves for navigation, patterns etc with resize handles. It's not a great name I think, well, it left me scratching my head at least. Reference: Resizable editor (with drag handles) in Site Editor #55122 (comment)
Yeah, the challenge for managing this state is that most, if not all, of these modes are not mutually exclusive - there are various combinations.
An editor could be in "zoom-out" mode (__unstableGetEditorMode) and "template-only" (renderingMode), is-outline-mode (postType === 'wp_template').
Then canvas mode, navigation/edit mode and whatever else.
|
Size Change: +855 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ramonjd
reviewed
Nov 24, 2023
| @@ -1241,6 +1253,19 @@ _Related_ | |||
|
|
|||
| - selectBlock in core/block-editor store. | |||
|
|
|||
| ### setRenderingMode | |||
There was a problem hiding this comment.
renderingMode is as good a name as any. I appreciate how you've combined pageContentFocusType values 👍🏻
There was a problem hiding this comment.
Same, it's a nice way of combining things! I wonder if it'd be worth making it a tiny bit more specific and using something like blockRenderingMode? Mostly I'm trying to think how can the name hint that this mode is about which blocks are displayed (and their locked status) 🤔
There was a problem hiding this comment.
I prefer slightly renderingMode over blockRenderingMode because blockRenderingMode feels like "block specific" while here we're just actually focusing on one entity over another for both content (blocks) but also things like sidebars, headers...
There was a problem hiding this comment.
Good point! Sounds reasonable to me 🙂
|
I've only smoke tested so far in the post and site editors: ✅ No regression in page editing in the site editor, including switching template preview on/off, swapping templates, template locking/flipping between template and page editing |
There was a problem hiding this comment.
I only had a chance to smoke test this a bit today, but I like the overall direction, too! Feels nicer to group together into a particular mode in the editor store 👍
One other quick thought: if we have a finite list of modes, is it worth using constants for the strings rather than hard-coding all or template-only, etc when calling setRenderingMode? Or does that get a bit awkward since each package would need to add their consts?
Happy to give this a more detailed test next week if it hasn't been merged by then 🙂
| @@ -1241,6 +1253,19 @@ _Related_ | |||
|
|
|||
| - selectBlock in core/block-editor store. | |||
|
|
|||
| ### setRenderingMode | |||
There was a problem hiding this comment.
Same, it's a nice way of combining things! I wonder if it'd be worth making it a tiny bit more specific and using something like blockRenderingMode? Mostly I'm trying to think how can the name hint that this mode is about which blocks are displayed (and their locked status) 🤔
What I don't like is that the package need to export these constants. I'm not strongly opinionated here and would be ok if added later though. |
ntsekouras
reviewed
Nov 24, 2023
ntsekouras
reviewed
Nov 24, 2023
ntsekouras
reviewed
Nov 24, 2023
ntsekouras
approved these changes
Nov 24, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related #52632
What?
This PR is a refactoring PR that moves the "template mode / page focus" modes state from the edit-site store to the editor store. The idea is that by moving the state and the components to the editor-store, we will be able to implement the same modes in the post editor as well.
The current PR only moves the "state" and doesn't touch the component. It should not have any impact on the behavior of these different modes in the site editor.
Testing Instructions
Check things like: "editing page", "switching to templates", "editing template parts"... in the site editor.