Edit site: remove empty preview border and redirect to editor in global styles navigation #67548
+24
−1
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
ramonjd
added
[Type] Bug
ramonjd
requested review from
jameskoster,
tellthemachines,
jorgefilipecosta and
t-hamano
|
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. |
6 tasks
|
Size Change: +86 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
t-hamano
reviewed
Dec 4, 2024
There was a problem hiding this comment.
Removes a weird and useless grey border
I'm wondering why the mobile layout doesn't show the iframe preview in the first place.
For example, if I switch from desktop to mobile layout, this preview shows up, but if I reload the browser this preview disappears:
df7bb5f232d8e7eab83af96799f536ac.mp4
Perhaps this part is related, but I don't understand why the preview doesn't show up 🤔
|
Thanks for looking into this @t-hamano
I was going to just remove that width check, but the comments seem to point to larger issue so I did it the CSS hacky way. Maybe we can disable that width check for the global styles panel 🤷🏻 cc @scruffian Removing the check would mean that the preview always shows I'm assuming |
|
I've figured out a bit what's going on. It doesn't seem to be due to the width check. In the Styles screen for mobile layout, the editor doesn't get rendered. That means the iframe itself doesn't get rendered because gutenberg/packages/block-editor/src/components/iframe/index.js Lines 376 to 391 in d017783 It works fine if we wrap it in Diffdiff --git a/packages/edit-site/src/components/global-styles/preview-iframe.js b/packages/edit-site/src/components/global-styles/preview-iframe.js
index e830accf6d..c40518d906 100644
--- a/packages/edit-site/src/components/global-styles/preview-iframe.js
+++ b/packages/edit-site/src/components/global-styles/preview-iframe.js
@@ -19,9 +19,11 @@ import { useLayoutEffect, useState, useMemo } from '@wordpress/element';
*/
import { unlock } from '../../lock-unlock';
-const { useGlobalStyle, useGlobalStylesOutput } = unlock(
- blockEditorPrivateApis
-);
+const {
+ ExperimentalBlockEditorProvider,
+ useGlobalStyle,
+ useGlobalStylesOutput,
+} = unlock( blockEditorPrivateApis );
const normalizedWidth = 248;
const normalizedHeight = 152;
@@ -115,37 +117,41 @@ export default function PreviewIframe( {
{ containerResizeListener }
</div>
{ isReady && (
- <Iframe
- className="edit-site-global-styles-preview__iframe"
- style={ {
- height: normalizedHeight * ratio,
- } }
- onMouseEnter={ () => setIsHovered( true ) }
- onMouseLeave={ () => setIsHovered( false ) }
- tabIndex={ -1 }
- >
- <EditorStyles styles={ editorStyles } />
- <motion.div
+ <ExperimentalBlockEditorProvider>
+ <Iframe
+ className="edit-site-global-styles-preview__iframe"
style={ {
height: normalizedHeight * ratio,
- width: '100%',
- .concat( children ) // This makes sure children is always an array.
- .map( ( child, key ) => child( { ratio, key } ) ) }
- </motion.div>
- </Iframe>
+ <EditorStyles styles={ editorStyles } />
+ <motion.div
+ style={ {
+ height: normalizedHeight * ratio,
+ width: '100%',
+ background: gradientValue ?? backgroundColor,
+ cursor: withHoverView ? 'pointer' : undefined,
+ } }
+ initial="start"
+ animate={
+ ( isHovered || isFocused ) &&
+ ! disableMotion &&
+ label
+ ? 'hover'
+ : 'start'
+ }
+ >
+ { []
+ .concat( children ) // This makes sure children is always an array.
+ .map( ( child, key ) =>
+ child( { ratio, key } )
+ ) }
+ </motion.div>
+ </Iframe>
+ </ExperimentalBlockEditorProvider>
) }
</>
); |
|
Thanks for digging in @t-hamano! What do you think, should I revert my CSS hack here and leave open a proper fix for another PR? I'm not sure I can get to it. |
I think I remember having a similar issue to this with the Patterns page, and that had to be wrapped with a That makes me wonder if it's better to wrap the whole 'view' rather than a specific component. |
t-hamano
approved these changes
Dec 4, 2024
There was a problem hiding this comment.
I think it's okay to ship this PR with the CSS hack for now.
That makes me wonder if it's better to wrap the whole 'view' rather than a specific component.
This is a good idea. As for the global styles preview for mobile layouts, it might be safer to address that in a follow-up.
Another thought: I don't understand why we need the Iframe component as a wrapper for the global styles preview in the first place.
The Iframe component doesn't render anything unless __internalIsInitialized in the block editor store is true, so we need the BlockEditorProvider provider, as shown here. But the global styles preview doesn't contain any blocks. All the data and styles needed for the preview should be available via the GlobalStylesProvider or the global styles context.
As a test, I made the following changes to not use the Iframe component, but the visual change is small, and I think it can be refactored with minor adjustments:
Diff
diff --git a/packages/edit-site/src/components/global-styles/preview-iframe.js b/packages/edit-site/src/components/global-styles/preview-iframe.js
index e830accf6d..4024cd19ae 100644
--- a/packages/edit-site/src/components/global-styles/preview-iframe.js
+++ b/packages/edit-site/src/components/global-styles/preview-iframe.js
@@ -115,7 +115,7 @@ export default function PreviewIframe( {
{ containerResizeListener }
</div>
{ isReady && (
- <Iframe
+ <div
className="edit-site-global-styles-preview__iframe"
style={ {
height: normalizedHeight * ratio,
@@ -124,7 +124,6 @@ export default function PreviewIframe( {
onMouseLeave={ () => setIsHovered( false ) }
tabIndex={ -1 }
>
- <EditorStyles styles={ editorStyles } />
<motion.div
style={ {
height: normalizedHeight * ratio,
@@ -145,7 +144,7 @@ export default function PreviewIframe( {
.concat( children ) // This makes sure children is always an array.
.map( ( child, key ) => child( { ratio, key } ) ) }
</motion.div>
- </Iframe>
+ </div>
) }
</>
);
|
Thanks for the investigation and help, folks 🙇🏻 |
michalczaplinski
pushed a commit
that referenced
this pull request
Dec 5, 2024
…al styles navigation (#67548) Co-authored-by: ramonjd <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: talldan <[email protected]>
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?
Some quality of life improvements for Edit Site styles navigation in mobile view.
p=/stylesdon't result in the user being stuck in the styles view without a WP logo or any way to exit.Why?
/wp-admin/site-editor.php?p=/styles&canvas=editnever opens the editor. That means, if I change from landscape to portrait in my mobile while viewing global styles in edit mode, I'm stuck on the styles screen with no way out.How?
Testing Instructions
/wp-admin/site-editor.php?p=/styles&canvas=edit.Screenshots or screencast
Before
The border 👎🏻
Kapture.2024-12-04.at.12.20.31.mp4
The editor 👎🏻
Kapture.2024-12-04.at.12.49.13.mp4
After
The border 👍🏻
Kapture.2024-12-04.at.12.20.52.mp4
The editor 👍🏻
Kapture.2024-12-04.at.12.50.23.mp4