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

Layout Grid: Editor Mobile View Not Fully Responsive #152

Closed
tjcafferkey opened this issue Oct 20, 2020 · 15 comments
Closed

Layout Grid: Editor Mobile View Not Fully Responsive #152

tjcafferkey opened this issue Oct 20, 2020 · 15 comments
Labels
Bug Something isn't working

Comments

@tjcafferkey
Copy link

There appears to be an issue in the Block Editor when viewing it on mobile where the responsive aspect of the view is broken.

Screenshot 2020-10-20 at 15 39 01

After some investigation as part of Automattic/wp-calypso#42148 it appears to be caused by some styles originating from this file 26dde-pb.

I am trying to find out why these styles exist and the problem they solve, to understand if there's a way we can fix this issue without effecting the original reason for their existence.

@jasmussen
Copy link
Member

Thanks for the report. 26dde-pb seems to be a minified name for a stylesheet, can you perchance include a snippet of the conflicting CSS directly here? Then I can investigate better.

@johngodley
Copy link
Member

It is the editor.css file

@johngodley
Copy link
Member

@tjcafferkey can you link to a post where this is happening, or copy/paste some HTML here to reproduce? I've tried the grid on an iPhone and it seems to work fine.

@tjcafferkey
Copy link
Author

tjcafferkey commented Oct 21, 2020

Hey @johngodley, here is a link to the page where it's happening, but here is the markup from the code editor perhaps you could replicate by creating a new page and importing this 26e0e-pb

@jasmussen
Copy link
Member

Thanks for the added info. I'm going to take a look at a patch as soon as I can.

@jasmussen
Copy link
Member

Alright I've taken a look, I thought the issue was with the responsive preview feature, but that appears to work as intended even though there are aspects of the layout that aren't ideal, and that there seems to be an issue with the media & text block not "stacking" as it should on mobile. That should be solved when the iframe lands.

layout test

However I as able to learn this:

resize

What happens there is that the layout grid block keeps showing the desktop breakpoint, even when the viewport is sized down. Which means that you see the full 12 column grid, even on tablet and desktop breakpoints that should be 8 and 4.

I can't recall if we support this, @johngodley do you recall? It would be nice to support, but I don't recall that we do.

@johngodley
Copy link
Member

No, there's no explicit code to change the editor to the actual breakpoint, and if that is the cause of the problem then I guess this is working as expected.

The block probably can change the editor breakpoint as the window is resized, but as this affects the entire editor it could be a little aggressive. Is this something that the core editor itself should do?

@jasmussen
Copy link
Member

The block probably can change the editor breakpoint as the window is resized, but as this affects the entire editor it could be a little aggressive. Is this something that the core editor itself should do?

I think this is something we can potentially optimize in the block when the iframe lands, as the iframe, I believe, is the core editors way of handling this.

@roo2
Copy link
Contributor

roo2 commented Dec 10, 2020

I did some investigation into this as part of this issue Automattic/wp-calypso#47293

The __experimentalGetPreviewDeviceType flag is being used to determine whether to display the editor in Desktop, or Mobile mode. edit.js
The __experimentalGetPreviewDeviceType flag seems to represent which device size is selected in the device preview window. But the code in edit.js isn't actually used for the device preview!

So I don't think it makes any sense to use __experimentalGetPreviewDeviceType in edit.js, I wonder if there is somewhere else to get the device type or device width even?

@johngodley
Copy link
Member

@roo2 I'm not sure I understand. __experimentalGetPreviewDeviceType is used to change which grid breakpoint is shown. What are your concerns with using it? How would using another method change things?

@roo2
Copy link
Contributor

roo2 commented Dec 11, 2020

@johngodley by the way that __experimentalGetPreviewDeviceType is used in edit.js, I expected it to set the breakpoint that the editor would simulate. So when I emulate an iphone, I expected __experimentalGetPreviewDeviceType to return "Mobile" and the editor to use the stacked layout.

But what happens is that when __experimentalGetPreviewDeviceType is used by edit.js#474, it always returns "Desktop", causing the desktop column layout to be rendered in the editor on mobiles.

Screen Shot 2020-12-11 at 10 58 22 AM

So I'm not sure of the correct fix, but it seems like within edit.js, the way we're using __experimentalGetPreviewDeviceType is not serving its intended purpose ( It seems to always return "Desktop" ).

@johngodley
Copy link
Member

__experimentalGetPreviewDeviceType refers to the preview device type (accessed from the preview dropdown), not the actual device type.

In terms of edit.js the preview device type is used to change the grid breakpoint, and so it is behaving as intended.

@roo2
Copy link
Contributor

roo2 commented Dec 14, 2020

ahh sorry yes, I had missed that part of the UI in the block settings 🤦‍♂️ that makes sense!

@roo2
Copy link
Contributor

roo2 commented Dec 16, 2020

I've raised #161 which is a potential solution to allow the editor to work intuitively for mobile and still support the current preview mode

@johngodley
Copy link
Member

Version 1.5 has been released which includes the fixes in #161

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants