-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Site Editor: add fullscreen close button #20989
Conversation
Size Change: +1.15 kB (0%) Total Size: 858 kB
ℹ️ View Unchanged
|
@@ -18,30 +18,31 @@ const wordPressLogo = ( | |||
</SVG> | |||
); | |||
|
|||
function FullscreenModeClose() { | |||
const { isActive, postType } = useSelect( ( select ) => { | |||
function FullscreenModeClose( { isActive } ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior of that button look completely different between the two editors. Do you think we should just copy/paste addapt?
- I see the
core/editor
store is used in block-editor which is not great. - isActive has been extractedd
- the target URL is different.
Maybe it does make sense as a small component in the upcoming @wordpress/interface
package but I wonder if it's a too early abstraction for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior of that button look completely different between the two editors. Do you think we should just copy/paste addapt?
Yes, I think it might make more sense to do that for now. We can consider abstracting it in @wordpress/interface
later on if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored as suggested in 488fa35.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well for me.
It seems a little odd to go back to 'index.php' in every case of closing the Site Editor from the fullscreen. But I can't really think of any better concrete landing point for that either.
Would it make sense to direct back to the last window location as the browsers back button would? Land folks closing the site editor into whatever section of wp-admin they were in when they decided to open it as opposed to always 'index.php' ? Or maybe even just to close fullscreen mode as the non-fullscreen site editor itself is the main landing point in this context where the page/post selection is for the existing editor? I guess these are more design questions, but index.php
is probably the best to go with for now...
I'm not sure if this is the best way to go about it, since someone could navigate directly to it via link or bookmark, and then clicking the close button would unexpectedly take them to their previous page which might be out of their WP site completely?
The problem with this and how it currently works is that it wouldn't be possible to have the fullscreen preference permanently on, since exiting the editor would always toggle it back. |
132345f
to
488fa35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make fullscreen default like the post screen?
43923cf
to
840703e
Compare
Yes, I think it now makes sense with latest UI additions. Updated in 840703e02fea4f02b1a51c74ce5cf4878f7e1051. |
840703e
to
db2e5e4
Compare
db2e5e4
to
58ce859
Compare
6dba089
to
96b6614
Compare
Description
Add Fullscreen mode close but to the Site Editor, similar to the one we already have in post editor.
Follow up of Follow up of #20691.
How has this been tested?
wp.data.dispatch( 'core/edit-site' ).toggleFeature( 'fullscreenMode' );
.Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: