Display the DocumentBar for Style Book and Style Revisions #62669
Conversation
creativecoder
added
[Package] Editor
|
Size Change: +86 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
| ); | ||
| useEffect( () => { | ||
| setEditorContentOverlayTitleAndIcon( title, icon ); | ||
| }, [ title, icon, setEditorContentOverlayTitleAndIcon ] ); |
There was a problem hiding this comment.
This basically replaces the "title" prop with an action API. Do you think it's a better API? Why?
If we're to keep this, I guess we should remove the "title" prop from the Editor component (and its children)
There was a problem hiding this comment.
@youknowriad Thanks for taking a look
This basically replaces the "title" prop with an action API. Do you think it's a better API? Why?
I did it this way because I was unsure of the backcompat restraints on the @wordpress/editor components, and this way keeps all API changes private.
Alternatively we could make the following modifications
- Add an
iconprop toEditor(from@wordpress/editor) so thatEditSiteEditorcan use it to override the icon (in addition to the title override it's using now) - Add title and icon props to the
DocumentBarcomponent to override the title and icon inferred from the currently open document or template, so that we can display the title and icon overrides fromEditSiteEditorinside the DocumentBar when an overlay is visible (rather than just replacing the component with a string, like we do now)
Given the plans to refactor EditorCanvasContainer for Style Book and Style Revisions I'm hesitant to add new props to @wordpress/editor components just for the sake of the site editor. On the other hand, those implementing custom editors might appreciate the title/icon override props, so it could make sense to keep them around. What do you think?
There was a problem hiding this comment.
To be honest, neither solution satisfy me completely :P
- I like the simplicity of the props but don't like the fact that they need to be passed down to the components.
- I don't like the complexity of the actions/selectors but they don't need to be passed down to components.
Worth nothing that the components we're talking about are also private AFAIK. So maybe for now, I have a small preference towards just adding an icon prop.
There was a problem hiding this comment.
To be honest, neither solution satisfy me completely ... So maybe for now, I have a small preference towards just adding an icon prop.
I feel similarly! If nothing else, using props is a little less code overall. I'll rework the PR to use props and we can see how that feels.
There was a problem hiding this comment.
By the way, DocumentBar is exported from @wordpress/editor, so adding props to the component does change the public API of the package.
|
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. |
| const commands = []; | ||
|
|
||
| if ( postType === PATTERN_POST_TYPE ) { | ||
| if ( ! hasEditorContentOverlay && postType === PATTERN_POST_TYPE ) { |
There was a problem hiding this comment.
When editing a page, there's an Edit Template command as well:
Do you think that should also be hidden?
There was a problem hiding this comment.
I'm not sure I understand the change here to be honest?
There was a problem hiding this comment.
Yeah, might need a comment.
When editing a pattern, and then activating the style book, this condition ensures the default pattern commands aren't shown by default when the command centre is opened:
There was a problem hiding this comment.
Is this the right fix? Maybe when going into "style book" and "style revision", we should be changing the "context" instead. (See useSetCommandContext) Again, I think this is just a side effect of us not actually changing the active "entity" in the EditorProvider to the global styles entity, but that is definitely a bigger change.
There was a problem hiding this comment.
Good points! I've updated to have the edit-site package remove the command context temporarily when Style Book and/or Style Revisions are open, clearing the suggested commands that are specific to editing the current entity (for both patterns and page): https://github.com/WordPress/gutenberg/pull/62669/files#diff-caaab4735e981969da0fb0108be1c6a34e2f039940615a695fa05e061dac42d9R40
Again, I think this is just a side effect of us not actually changing the active "entity" in the EditorProvider to the global styles entity, but that is definitely a bigger change.
Agreed, I think this is another example supporting how opening a global styles entity would make more sense. But hopefully this workaround is acceptable for now, until we've made that much bigger change.
| : undefined | ||
| } | ||
| title={ title } | ||
| icon={ icon } |
There was a problem hiding this comment.
Using the ! hasDefaultEditorCanvasView check here causes the DocumentBar title/icon to flash back and forth between the document and overlay titles when activating the Style Book in addition to Style Revisions and vice versa.
Since the default title and icon are empty strings and the return value of getEditorCanvasContainerTitleAndIcon is directly tied the canvas container state, the check isn't really needed.
| /* | ||
| * The editor canvas overlay will likely be deprecated in the future, so for now we clear the command context | ||
| * to remove the suggested commands that may not make sense with Style Book or Style Revisions open. | ||
| * See https://github.com/WordPress/gutenberg/issues/62216. |
There was a problem hiding this comment.
So, in the future we'd have something like commandContext = getCurrentSiteEditorEntity() or something?
Anyhoo, I'm going to try to shuffle some things around and start looking at that issue after 6.6. When the time comes, it'd be great to your feedback on the possible approaches 🙇🏻
There was a problem hiding this comment.
So, in the future we'd have something like commandContext = getCurrentSiteEditorEntity() or something?
I don't think a new command context would be needed. Presumably if opening Style Book was changing the currently edited entity to a wp_global_styles post, then the check here could just be removed. The command context would go back to being entity-edit with the wp_global_styles post type returned by getCurrentPostType from the editor store, which should cover any checks needed for adding suggested commands.
For example, in the case of the suggested pattern commands I was manually checking before, the post type is no longer wp_pattern, so those would be hidden when Style Book is open without any additional changes.
There was a problem hiding this comment.
When the time comes, it'd be great to your feedback on the possible approaches 🙇🏻
Yes, for sure! Putting together this PR was a great crash course on understanding the current relationship between the editor and edit-site packages--I'd be glad to put that knowledge to more use.
creativecoder
force-pushed
the
try/document-bar-for-style-book-and-revisions
branch
from
27c0425 to
4000088
Compare
|
I'm not sure what's going on with CI: phpunit tests look green but it's not coming through here in the PR. |
|
@creativecoder Tests have been failing frequently lately. If you're confident the fails are not related — which has been the case in the last week for many committed PRs — then I think it should be fine. Especially if this PR isn't to be backported to the 6.6 RC. |
|
Thanks @ramonjd ! In this case everything was passing, but the test status wasn't coming through to the PR. Looks like it's working again now. I'll run everything again, just to be sure, then I'll get this merged. |
|
Thank you, @creativecoder! |
|
Leaving myself some notes, for when I come back to any related issues: The DocumentBar component in the header, used to show the icon/title and trigger the command center modal, lives in a component hierarchy like this
The DocumentBar usually shows the title and icon from the entity currently being edited (post, template, etc), and now has The In the future (see #62216), Style Book and Style Revisions will likely open the current |
What?
When viewing Style Book or Style Revisions in the site editor, displays the DocumentBar in header rather than just the title.
Fixes #51667 and #54514
Why?
Makes the header more consistent. Opening the Style Book or Style Revisions overlay now only changes the title and icon, rather than removing the DocumentBar component and replacing it with a string.
How?
Given the plans for refactoring and/or removing the EditorCanvasContainer component, this PR attempts to make the minimal changes needed to solve the issue
titleandiconprops toDocumentBarso it can display them when an overlay is visibleHeadercomponent to passtitleandiconprops toDocumentBar, rather than displaying thetitleprop in place ofDocumentBarTesting Instructions
Testing Instructions for Keyboard
No relevant changes
Screenshots or screencast
Screen.Recording.2024-06-18.at.20.26.54.mov