Site and Post Editor: Unify the DocumentBar component #56778
Conversation
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
jameskoster,
jasmussen,
ndiego,
jorgefilipecosta and
a team
| @@ -17,8 +17,6 @@ const STORE_NAME = 'core/commands'; | |||
| /** | |||
| * Store definition for the commands namespace. | |||
| * | |||
| * See how the Commands Store is being used in components like [site-hub](https://github.com/WordPress/gutenberg/blob/HEAD/packages/edit-site/src/components/site-hub/index.js#L23) and [document-actions](https://github.com/WordPress/gutenberg/blob/HEAD/packages/edit-post/src/components/header/document-actions/index.js#L14). | |||
There was a problem hiding this comment.
Removed this because it gets out of sync very quickly as the code evolves.
There was a problem hiding this comment.
We could replace the reference to specific instances with a search query like https://github.com/search?q=repo%3AWordPress%2Fgutenberg+commandsStore+language%3AJavaScript&type=code&l=JavaScript which should always return relevant results.
|
Size Change: +115 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d1eb313. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7114315608
|
|
Looks good at a glance. One nuance, the document bar was recently made to be 32px tall, like so:
In this branch it's 36 again:
You can use $button-size-compact. |
|
The variable was correct but the buttons didn't have the compact prop. |
youknowriad
force-pushed
the
update/merge-document-bar
branch
from
6baa7ae to
593b841
Compare
|
I've noticed three things.
trunk:
This PR:
Third, when selecting a template part, the title seems to be incorrect. 907eff58820058c6c394051021dcb588.mp4 |
|
@t-hamano for the color, it was a deliberate choice from me because I felt it was a bit out of place there and random. For the other issues, they should be fixed now. |
| postId | ||
| ); | ||
| const { templateIcon, templateTitle } = useSelect( ( select ) => { | ||
| const { __experimentalGetTemplateInfo: getTemplateInfo } = |
There was a problem hiding this comment.
I hate this selector, it's so bad in so many ways, but it's what we have today. Let's keep changes to that for later.
There was a problem hiding this comment.
Just curious, how do you think it could be improved?
There was a problem hiding this comment.
- Well first it says
getTemplateInfoand it works both for templates and template parts - I don't see why we need something like that specific to these post types but can't include all post types (to avoid the isTemplate conditionals in this component for instance)
- It takes an object, why? It should take postType, postId. A selector taking an object like that is hard to memoize... The object is probably changing way too often.
- It is also misplaced, I'm not sure why this is the "editor" package, it should be something for "core-data" maybe.
| @@ -17,8 +17,6 @@ const STORE_NAME = 'core/commands'; | |||
| /** | |||
| * Store definition for the commands namespace. | |||
| * | |||
| * See how the Commands Store is being used in components like [site-hub](https://github.com/WordPress/gutenberg/blob/HEAD/packages/edit-site/src/components/site-hub/index.js#L23) and [document-actions](https://github.com/WordPress/gutenberg/blob/HEAD/packages/edit-post/src/components/header/document-actions/index.js#L14). | |||
There was a problem hiding this comment.
We could replace the reference to specific instances with a search query like https://github.com/search?q=repo%3AWordPress%2Fgutenberg+commandsStore+language%3AJavaScript&type=code&l=JavaScript which should always return relevant results.
| postId | ||
| ); | ||
| const { templateIcon, templateTitle } = useSelect( ( select ) => { | ||
| const { __experimentalGetTemplateInfo: getTemplateInfo } = |
There was a problem hiding this comment.
Just curious, how do you think it could be improved?
| @@ -108,17 +93,17 @@ | |||
| position: absolute; | |||
There was a problem hiding this comment.
I know it's already like this in the site editor, but this absolutely positioned button makes the focus ring for the command palette button take up the whole bar:
which is a bit confusing because it doesn't correspond to the effective clickable area.
There was a problem hiding this comment.
Yes, that's a design question that I'll leave out of this PR, the problem is that we also want to change the whole color on hover.
|
This could be addressed as a follow-up, but I noticed an issue with the document bar hiding the main inserter in the mobile viewport. This issue also occurs with trunk, but not with WP6.4. trunk: trunk.mp4WP6.4: wp6.4.mp4 |
Co-authored-by: Aki Hamano <[email protected]>
| }; | ||
| }, [] ); | ||
| const archiveLabels = useArchiveLabel( templateSlug ); | ||
| const defaultRenderingMode = useMemo( () => { | ||
| return postWithTemplate ? 'template-locked' : 'all'; |
There was a problem hiding this comment.
Bad habit. Thanks for noticing
|
Actually no, but it might be the result of the PR that introduce that regression as well. |
@youknowriad Distinguishing global vs local editing is a big pain point. We do not something that indicates the scope that you are editing. Until we find a better way (being discussed here), are we able to leave the colour for now? |
Sure I can restore it but it does feel very out of place, I'm curious to know how many people make that connection if I'm not even capable of doing it. |
| isEditingTemplate | ||
| ? () => | ||
| setRenderingMode( | ||
| getEditorSettings().defaultRenderingMode | ||
| ) |
There was a problem hiding this comment.
Why is this limited only to template types? Navigation Menu posts (wp_navigation) are also edited in this context and it seems like "back" default rendering mode would be useful here also. I'm just checking whether this was intentionally locked down to templates only.
There was a problem hiding this comment.
Oh I see, this is about rendering modes.
There was a problem hiding this comment.
How would you feel about the default "back" action always be to go "back" in the @wordpress/router history?
eg. history.back()
Design would like this "Back" button to become the default way to go back out of the template editing mode and I need to implement this in #55657.
There was a problem hiding this comment.
The post editor doesn't use the router, so this would break.
There was a problem hiding this comment.
I also feel like "back" using "history.back" is a bit redundant with the browser's back button. This "back" is more a "exist template mode" button.
There was a problem hiding this comment.
Thanks for going over this again Joen. So hopefully we can agree that UI-wise we'd like the "Back" in the top toolbar to become the default UI control for that action?
@youknowriad Technically, could we add a referrer prop to the URL which we can then consume to determine the best location for "back". If none is provided we optimise for "go back to the main site editor view"?
Obviously there would need to be an exception for situations like what is currently handled in this PR where "Back" means "Exit template editing mode" but that's simple enough. What do you think?
There was a problem hiding this comment.
To be honest, it's not satisfying for me. I think we're breaking a big conceptual element in order to satisfy a small affordance.
Think of the whole editor or frame as <Editor postType={ currentPostType } postId={ currentPostId } />. So the whole editor is just a "reusable component" that you can use in any context (post editor, site editor, elsewhere...).
Also the editor understands "template modes" so you can do <Editor postType={postType} postId={postId} templateId={templateId} /> and all the switching of modes (from post+template to template only and back...) is all internal. So in that sense, a "back" button within the editor to switch between these modes make sense.
Now, think of focus modes: focus modes mean switching the actual "postType" and "postId" passed to the editor, so they are things that are external to the editor. It's a shame to have to update the editor internally for this affordance. Also and more imporantly, it might not make sense in all these "editors" and "frames". For me, "focus mode" means navigating to another entity in the site editor, you actually switch a lot of things, not just the UI of the editor, you switch the active page in the sidebar of the site editor, you switch the URL..., you're not within the same editor anymore, it's another one. So it feels weird to have a back button within the editor. For me it's a cross page affordance and in that sense is better placed outside the editor.
Now, that is my opinion and my strong opinion here is that we shouldn't break the Editor abstraction just for this. That said, If you all feel very strongly about this "back" button, we can try to find a compromise and have a "setting" in the settings passed to EditorProvider that contains the "back referrer" or something like that. Maybe even make it a private setting (private API)
But I persist, that I think it's wrong personally :)
There was a problem hiding this comment.
Can we agree that when users enter focus mode for a template part they need to be given an easy way back to the template they were editing?
There was a problem hiding this comment.
better placed outside the editor
Do you mean literally? The idea of a notice bar at the bottom of the viewport has come up a few times but never really gained much traction.
Worth noting theres a 'Back to x' suggestion in the command palette when you click the document bar, so I'd agree with Joen – the current placement feels natural in terms of UX.
There was a problem hiding this comment.
After discussing more with @jasmussen It seems the main point of friction is that:
- we have "focus mode" for template part, navigation block and things like that where in fact we actually navigate to another page of the site editor.
- we have "template mode" in the site editor where we show a template, but we don't call that "focus mode" and we don't navigate to another page of the site editor.
If we are to treat the "back" button as similar for both these cases, they should first work similarly. In other words, maybe the "template mode" should be removed and instead we should navigate to the "template page" of the site editor instead.
Also, if we do that, and since the "template mode" is also an affordance of the "post editor", we should make sure to also add some kind of "template page" to the post editor too (maybe no url navigation there or anything but at least it should be something in the "state" of the post editor and the post editor should be able to switch).
Related #52632
What?
This PR updates the post and site editors, to use the exact same
DocumentBarcomponent and replaces bothDocumentActionscomponent that are visible in the site and post editors.To be able to do so, this PR had to switch the "template mode" of the post editor to only show the "template" without the edited post and page which matches the behavior of the site editor. (as discussed in the issue)
There are still some differences between how the modes work that I didn't update in this PR.
I left this consideration separate.
I also introduced a new
defaultRenderingModesetting to theEditorProviderto define the default mode that the editor needs to be in when initially rendering the current post/page/template...Testing Instructions
1- Check the template mode in the post editor, notice that it only shows the template now.
2- Check the document bar is shown properly while editing different entities in post and site editors.