Styles Screen: Ensure variations previews will render in mobile viewports #51080
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
added
[Type] Bug
andrewserong
requested review from
noisysocks,
apeatling,
ramonjd and
tellthemachines
|
Size Change: +58 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
ramonjd
reviewed
May 30, 2023
| onChange={ noop } | ||
| onInput={ noop } | ||
| > | ||
| <div className="edit-site-sidebar-navigation-screen-global-styles__content"> |
There was a problem hiding this comment.
Just checking, is the <div /> necessary here too?
There was a problem hiding this comment.
It's not necessary, but I copied the same approach as over in the Navigation screen, because if we're wrapping components within content in this screen, it felt like it might be good to have a container specific to this screen, particularly because we'll likely be adding other components to this screen in follow-ups (e.g. revisions potentially).
Happy to remove it for the moment if you don't think it's warranted for this PR! 🙂
andrewserong
deleted the
fix/styles-screen-display-variations-previews-on-mobile
branch
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Part of #50429
Fix a bug where on mobile viewports, the style variations previews were not rendering in the browse mode Styles screen. Note — this bug would only occur if you're always in a mobile viewport size for the duration of your visit to the site editor. If the editor canvas is at all visible, then the style variations will render, so it's an easy bug to miss when testing in a desktop browser.
Why?
It turns out that for the
Iframeblock editor component to successfully render aniframeelement, it performs a check that the block editor settings are initialised (here). This internal state (__internalIsInitialized) is set here within theBlockEditorProvider.In desktop screen sizes, this setting appears to be set already when accessing the Styles screen because the editor canvas has already been rendered, so an editor provider is already in use. However, in mobile viewports, the editor canvas isn't rendered yet, so the state isn't available yet.
Since the previews should have access to settings, etc, I borrowed a similar approach from the Navigation screen introduced in #50840, and added a
BlockEditorProviderto the global styles screen in browse mode.How?
<StyleVariationsContainer />in the Styles screen in aBlockEditorProvider.Testing Instructions
Screenshots or screencast
The below screengrabs depict reloading the Styles screen in the site editor browse mode in a mobile viewport size.