Skip to content
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 - prevent loading state from showing the admin menu. #36455

Merged
merged 4 commits into from
Nov 15, 2021

Conversation

Addison-Stavlo
Copy link
Contributor

Description

Resolves #36445 by ensuring the site editor is in fullscreen mode while loading.

How has this been tested?

Load the site editor and verify you do not see the wp-admin menu while it loads, as is described in the linked issue.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@Addison-Stavlo Addison-Stavlo requested review from jeyip, mattwiebe, vindl, Copons, creativecoder and youknowriad and removed request for Copons November 12, 2021 17:23
@@ -31,6 +31,15 @@ function gutenberg_is_edit_site_page( $page ) {
return 'appearance_page_gutenberg-edit-site' === $page;
}

// Default to is-fullscreen-mode to avoid rendering wp-admin navigation menu while loading and
// having jumps in the UI.
add_filter(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be executed for all pages, not just the site editor (because the php file is always loaded), should we instead move this into gutenberg_edit_site_init ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, that makes sense. I moved it into that function here c434238

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to also remove the filter as part of the ! gutenberg_is_edit_site_page( $hook ) conditional return?

Copy link
Contributor Author

@Addison-Stavlo Addison-Stavlo Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also curious, does the function need to be static here? I didn't think so, but then I noticed your example in wordpress-develop was also static but didn't seem to be part of a class?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they started using "static" for closures like that recently something about PHP 8 support, but I didn't fully follow the explanations tbh :) So I might think it's better if it's static here as well.

It doesn't seem like we need to remove the filter though as it's only applied if we're on this page so, so it looks good to me.

@skorasaurus skorasaurus added the [Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") label Nov 13, 2021
@Mamaduka Mamaduka added the [Type] Bug An existing feature does not function as intended label Nov 15, 2021
Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Thanks, @Addison-Stavlo.

@Addison-Stavlo Addison-Stavlo merged commit 600b1d6 into trunk Nov 15, 2021
@Addison-Stavlo Addison-Stavlo deleted the fix/site-editor-loading-screen-showing-menu branch November 15, 2021 12:59
@github-actions github-actions bot added this to the Gutenberg 12.0 milestone Nov 15, 2021
@Addison-Stavlo
Copy link
Contributor Author

Would we be able to get this into a point release for 11.9 if one ends up happening? cc @noisysocks

andrewserong pushed a commit that referenced this pull request Nov 16, 2021
)

* start with is-fullscreen-mode class on the body

* only add filter if loading edit site page

* remove unnecessary static prefix

* Update lib/full-site-editing/edit-site-page.php
@andrewserong
Copy link
Contributor

@Addison-Stavlo I'm not sure if there are plans for a point release, but I've cherry picked this PR into the release/11.9 branch in 6229382, so if anyone does do a point release, it'll be included.

@Addison-Stavlo
Copy link
Contributor Author

Thanks @andrewserong !

noisysocks pushed a commit that referenced this pull request Dec 13, 2021
)

* start with is-fullscreen-mode class on the body

* only add filter if loading edit site page

* remove unnecessary static prefix

* Update lib/full-site-editing/edit-site-page.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Editor Related to the overarching Site Editor (formerly "full site editing") [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(Site) "Editor (beta)" loads with wp-admin navigation in the background.
5 participants