Site Editor: add Fullscreen mode #20691
Conversation
vindl
requested review from
noahtallen,
Addison-Stavlo,
epiqueras and
youknowriad
|
Size Change: -7.31 kB (0%) Total Size: 857 kB
ℹ️ View Unchanged
|
packages/edit-post/src/index.js
Outdated
| @@ -155,3 +155,4 @@ export { default as PluginPostStatusInfo } from './components/sidebar/plugin-pos | |||
| export { default as PluginPrePublishPanel } from './components/sidebar/plugin-pre-publish-panel'; | |||
| export { default as PluginSidebar } from './components/sidebar/plugin-sidebar'; | |||
| export { default as PluginSidebarMoreMenuItem } from './components/header/plugin-sidebar-more-menu-item'; | |||
| export { default as FullscreenMode } from './components/fullscreen-mode'; | |||
There was a problem hiding this comment.
We were talking about this recently in another PR. I believe we shouldn't export this from edit-post.
We were considering creating a @wordpress/admin-screen package for reusable stuff across WP Admin screens.
it can include "Page/EditorSkeleton", "FullscreenMode", Potentially sidebar plugins...
There was a problem hiding this comment.
We were considering creating a @wordpress/admin-screen package for reusable stuff across WP Admin screens.
Cool, that definitely makes more sense and it was just the thing I was looking for. Is there an open issue/pr for it that I can track and update this when it's ready?
There was a problem hiding this comment.
There's none. @jorgefilipecosta might be working on a PR and you can also add the package as part of this PR 🤷♂
There was a problem hiding this comment.
We have a PR with admin screen available at #20698.
There was a problem hiding this comment.
While we figure out the best naming... for this package, I think we can land this as an experimental component in block-editor for now (like the skeleton)
There was a problem hiding this comment.
While we figure out the best naming... for this package, I think we can land this as an experimental component in block-editor for now (like the skeleton)
@youknowriad I moved this component to block-editor package as suggested. So far its activation state has been persisted in edit-post store. I factored that out from the component itself so we can have a separate state in case of edit-site.
vindl
force-pushed
the
add/site-editor-fullscreen-mode
branch
from
57ba4cd to
f683cca
Compare
vindl
force-pushed
the
add/site-editor-fullscreen-mode
branch
from
f683cca to
08067f3
Compare
packages/edit-post/src/style.scss
Outdated
| @@ -1,6 +1,6 @@ | |||
| $footer-height: $button-size-small; | |||
|
|
|||
| @import "./components/fullscreen-mode/style.scss"; | |||
| @import "../../block-editor/src/components/fullscreen-mode/style.scss"; | |||
There was a problem hiding this comment.
@youknowriad I'm thinking these styles should probably be moved out of block-editor and every editor implementation should be able to specify their own styles when is-fullscreen-mode class is added by FullscreenMode component. Since both edit-post and edit-site are running in the same wp-admin context, we'd probably end up duplicating the code from it, but that might be fine. What do you think?
There was a problem hiding this comment.
I agree with the moving, for the global style, maybe it's fine to just pick one as a convention.
There was a problem hiding this comment.
Based on our Slack discussion I moved this import to block-editor package in 9e2b8de.
This is needed to persist fullscreen mode activation independently of edit-post package.
vindl
force-pushed
the
add/site-editor-fullscreen-mode
branch
from
08067f3 to
081d4b5
Compare
There was a problem hiding this comment.
This works great on my end. Testing both the site editor and page/post editor fullscreen modes.
The code looks good too, I just had 1 question about a function there.
I'm kind of surprised there had yet to be a store set up for the edit-site package yet. But this is a very thorough start that should be easy to add onto in the future!
| * | ||
| * @return {Object} control descriptor. | ||
| */ | ||
| export function select( storeName, selectorName, ...args ) { |
There was a problem hiding this comment.
Where does this export get used?
I see the default export in this file gets used in registerStore in the index, but what is this function doing?
There was a problem hiding this comment.
I copied over the scaffolding from edit-post store to speed things up so likely an unintended leftover. Will recheck and remove it if it's not necessary. I also have to look into failing CI tests.
| @@ -12,6 +12,7 @@ import { render } from '@wordpress/element'; | |||
| * Internal dependencies | |||
| */ | |||
| import './hooks'; | |||
| import './store'; | |||
There was a problem hiding this comment.
I feel this config should be reused somehow as well, some options:
- Move it to the block-editor store (doesn't make sense for me as it's just a temporary location for the component)
- Use a store in the new package, downside is that it makes it a singleton package (another one). (option we used everywhere for the moment)
Options we didn't use anywhere yet but could be good for reusabliity
- Export a store config from the new package and leave the responsibility to register it to the caller
wp.data.registerStore( 'core/edit-post-full-screen', fullscreenStoreConfig ) - Don't use a store, use a React context provider.
I don't think it needs to be solved in this PR, but something to think about.
cc @aduth if you have thoughts.
There was a problem hiding this comment.
To clarify, by "config", do you mean the user's general preference of "full site mode" being activated or not, across different implementations of the block editor?
There was a problem hiding this comment.
yes. But not necessarily the persisted preference since the persisted one can be different from screen to other. (just the memory handling of the preference)
Thanks for the reviews and testing @youknowriad! I'll look into these next. |
Description
A quick experiment that attempts to reuse existing FullscreenMode component in Site Editor. In order to achieve this I've moved this component to
block-editorpackage so that it can be reused byedit-postandedit-site(as suggested by @youknowriad here).Note: For now I didn't want to add UI for toggling (similar to post editor ellipsis menu) until the designs for the Site Editor are ready. We'd like to force the fullscreen mode on Site Editor in our integration, and this should be enough to achieve that.
How has this been tested?
I tested the exiting post editor to make sure that there are no regressions to existing functionality.
In case of Site Editor, since there is still no UI for this, you can toggle the Fullscreen mode by running
wp.data.dispatch( 'core/edit-site' ).toggleFeature( 'fullscreenMode' )in the console.I also carried over the relevant
featureToggletests fromedit-postpackage here.npm test packages/edit-site/src/store/test/Screenshots
Types of changes
New feature (non-breaking change which adds functionality).
Checklist: