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

Fix Media & Text block preview alignment on larger devices #67097

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

im3dabasia
Copy link
Contributor

@im3dabasia im3dabasia commented Nov 19, 2024

What?

Ensures the Media & Text block displays side by side in preview mode, even on larger devices. Fixes ( #67079 )

Why?

Fixes an issue where the Media & Text block stacked in the preview on large screens due to a small preview container.

How?

I've applied the extra CSS only when the ancestor class is block-editor-block-preview__content-iframe, which is present only during the preview. This change will not affect the existing behavior of the Media & Text block's actual output on either small or large screen devices.

Testing Instructions

  1. Create a cover block
  2. Assign an image and add some text on top
  3. Try to convert the block into a Media & Text block, but only hover the option.

Screencast

Before:

Screen.Recording.2024-11-19.at.11.26.30.AM.mov

After:

Screen.Recording.2024-11-19.at.11.25.00.AM.mov

…vices

Ensured the Media & Text block displays components side by side in the preview, even on larger devices. The preview container was previously too small, causing the elements to stack. This change ensures the components remain properly aligned in the preview.
Copy link

github-actions bot commented Nov 19, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: im3dabasia <[email protected]>
Co-authored-by: michalczaplinski <[email protected]>
Co-authored-by: ellatrix <[email protected]>
Co-authored-by: dsas <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 19, 2024
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @im3dabasia! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@dsas dsas added the [Type] Bug An existing feature does not function as intended label Nov 22, 2024
@dsas
Copy link
Contributor

dsas commented Nov 22, 2024

Thanks for picking this up @im3dabasia

I've taken it for a test and it works great on the desktop view, however it introduces a problem on the mobile view.

Using the mobile view, the preview shows media and text side-by-side but both the site editor, and the front end of the site show image above media. This is a regression from the current status quo.

Screen.Recording.2024-11-22.at.14.46.52.mov

@@ -54,6 +54,11 @@
word-break: break-word;
}

.block-editor-block-preview__content-iframe .wp-block-media-text > .wp-block-media-text__content {
grid-column: 2 !important;
grid-row: 1 !important;
Copy link
Member

Choose a reason for hiding this comment

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

If we have to use !important, it doesn't seem like the right fix.

@ellatrix
Copy link
Member

Isn't it just a matter of making sure the preview width is larger? I see it's already set at viewportWidth. Why is that broken?

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

It looks like the width of the block switcher previews is hardcoded at 500. I think we should get the widths of the examples in the block type, and use the largest from those, then maybe fall back to the same width used for the inserter previews.

@im3dabasia
Copy link
Contributor Author

Hi @ellatrix,

I noticed that the preview isn't displayed on devices with a width smaller than 782px (i.e., mobile and tablet devices). Would it make sense to adjust the default viewportWidth from 500px to something more fitting, such as 1200px? It seems like a logical adjustment, especially since previews aren’t shown on medium screens or smaller.

Looking forward to your thoughts.

export default function PreviewBlockPopover( { blocks } ) {
const isMobile = useViewportMatch( 'medium', '<' );
if ( isMobile ) {
return null;
}
return (
<div className="block-editor-block-switcher__popover-preview-container">
<Popover
className="block-editor-block-switcher__popover-preview"
placement="right-start"
focusOnMount={ false }
offset={ 16 }
>
<div className="block-editor-block-switcher__preview">
<div className="block-editor-block-switcher__preview-title">
{ __( 'Preview' ) }
</div>
<BlockPreview viewportWidth={ 500 } blocks={ blocks } />
</div>
</Popover>
</div>
);
}

@michalczaplinski
Copy link
Contributor

How were you able to trigger the preview to show up @dsas ? Much like @im3dabasia I also notice that the previews don't appear in the editor if the viewport is less than 782px.


@im3dabasia I think that this won't work quite well. I gave it a try and while the Media & Text block looks better, some other blocks look worse - especially ones that include text because it becomes too small to read. This is using the width of 800px, with 1200px it would look even worse.

Before After
Screenshot 2025-01-02 at 12 27 46 Screenshot 2025-01-02 at 12 24 54

I think something like the approach outlined by @ellatrix makes sense here and should work for all blocks.

@michalczaplinski
Copy link
Contributor

The breakpoint for the Media & Text is $break-small which is 600px. If the viewportWidth is below that value, the image and text stack on top of one another.

For now, I think it's acceptable to set the viewportWidth to 601px to account for this. This makes the Media & Text behave correctly and it ALSO keeps the other blocks legible which was not the case with larger values.

I've implemented those changes in the PR now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants