-
Notifications
You must be signed in to change notification settings - Fork 18
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: Enable mobile rendering in mobile editor. #161
Conversation
Thank you for the PR. If I understand this correctly, this will "just work", correct? I.e. if you start on a mobile device and edit a layout grid, you'll edit the mobile breakpoint. If you start on a tablet, you'll edit the tablet breakpoint, etc — and the preview dropdown works as intended? If yes, then 👍 👍 on this, I'll just defer to John on a sanity check. |
Thanks for looking at this. In terms of the code, it seems a little fragile to be directly checking the browser width for specific pixel value break points. There are Gutenberg functions to monitor device widths and as Joen mentions in #152 there is functionality landing in Gutenberg that could help. I may be misunderstanding this PR, or maybe it's not working for me, but it seems to force the display to always match the current device, and the device preview dropdown has no effect other than changing the edit area. Viewing on a tablet in desktop mode incorrectly shows the tablet view: Viewing on a tablet in mobile mode incorrectly shows the tablet view, but on a smaller edit area: The idea with the preview detection is that on a real device it allows you to view and edit the grid of another device type, and as you change the preview mode that is reflected in the editor both in terms of the edit area and also the grid itself. If a mobile device cannot view the desktop mode then we should probably:
|
thanks for the follow up @johngodley! Different versions of gutenberg!Ahhh I see what's happened, I've only tested this against gutenberg
But in gutenberg
I had been targeting Why I think this is still worth implementingYea this definately also brings up an issue about potential further changes to gutenberg markup breaking this. I would make the case that this is still worth implementing because:
WRT preview of desktop from mobile:
I think that we're best to remove the desktop preview from mobile for now, because what you're seeing is the current behavior and it would take additional effort to make it work in desktop and it would be good to get design input on how previewing a larger device would work. Better way to get the device width from gutenberg's APIsI'll have to look into this and find out what the plans are in gutenberg WRT the iframe or other API methods? |
That's working better now, thanks. I'm still wary of the direct element querying and hardcoded breakpoint widths. It also probably might not work outside of the core post editor. For example, on P2. There is the hook Would removing the option from mobile devices negate the need for these changes on devices other than mobile? It seems like this is the only breakpoint that could have problems, and it's dependant on the content. There's some inconsistency in that the preview never matches the display until the user changes it. On tablet showing the tablet mode, but the preview shows desktop: On mobile showing the mobile mode, but the preview shows desktop: I don't know how the preview mode should work, but I wonder if it should default to the current device? That is, if I'm on a tablet then For example, if I'm on a tablet and click the preview button it shows I'm in desktop mode: I would expect that I'm in tablet mode when on a tablet. If I do change to tablet mode a grey bar appears: @jasmussen you may have more context here. |
This actually touches on a larger issue. The dropdown is definitely a desktop first affordance, allowing you to emulate the smaller breakpoints on the large screen, and make adjustments for that This mindset includes the principle that through good defaults, most blocks won't need responsive edits, and simply work well from the start. Where it gets tricky is blocks that have built-in responsiveness like this, defaulting to one breakpoint even on the smaller devices. I think we should focus on mobile as the primary challenge here, because what happens on mobile (<600px) is that the preview button becomes a single action that opens in a new tab. So we can probably just detect: are we mobile or not, and then ignore the tablet/desktop stuff for now — it seems acceptable on a big comfy ipad to be able to choose both desktop and tablet breakpoints, even if you're technically using a tablet — it could be a 13 inch one after all. This button to the rescue: So can we simply detect that we are on mobile, <600px, we default to the mobile breakpoint for the layout grid? |
Yep, that's what this PR will do, although it will also affect tablet and the preview dropdown will not initially match the displayed mode (i.e. the dropdown shows desktop, but the block shows tablet). If we hide the block device selection from mobile, and the preview doesn't show these anyway, then tablet and desktop can continue to behave as they currently do and will be consistent with the preview dropdown. My question about the behaviour of the preview dropdown is if I'm on a tablet and press the dropdown, would I expect the default to show desktop or tablet? Currently it shows desktop, even though I'm on a tablet. I can raise this in the Gutenberg repo if you think it's worth looking at? If the default preview mode matched the current device then this PR could be simplified. |
hey guys, I've got some more thoughts to add Mobile preview mode isn't ideal on all mobiles
hmmmm currently the way that mobile preview is implemented, it will constrain the screen width to 360px, this would be strange on e.g. a 414px wide mobile device. We get this result: ( gray spaces down the sides ) Rename "desktop" to default?What "Desktop" display mode is actually doing is removing the width restrictions and using all available screen space. That's actually the default behavior that we would want on mobile. I think the problem is the name "Desktop". I think it could be renamed to "Default" to match what the button is actually doing? Only preview smaller devicesAlso yea, as you said I think it doesn't really make sense to offer larger previews than what the screen size can actually support. So I'd propose that: |
Yep, good thoughts. What I'm thinking is that we should explore some changes in the block editor directly (core) of renaming the desktop and tablet breakpoints to something else. They still make sense to design for, but for the reasons outlined, they aren't really representative. In the meantime, I think we can move this PR forward by focusing on only mobile. Specifically, we make no change to current tablet/desktop breakpoints — we decide that it's okay that you see the desktop breakpoint on tablets. Do we remove the toolbar button to edit responsively from the mobile breakpoint? I don't know — if we default to showing the mobile breakpoint on mobile, it seems fair to keep that button, even if it affects the entire viewport. We can then look at upstream improvements. |
Phew I was having a think about it this morning, @jasmussen, @johngodley I do see the fundamental problem with this approach: We really do want to keep the preview mode and rendering mode the same so that the column span/offset settings selected match what is rendered in the editor. So I had a think about it and ( as you suggested ) I think there's a good way to solve this in core I did a pretty hefty write up of what I'm thinking in this issue: Thanks for your inputs so far! |
Thank you @roo2. I wrote some thoughts on that thread. I can't help but feel like there's an aspect I'm missing, but at the current moment the biggest challenge I see is that in the case of Layout Grid, any edit made is tied to the specific responsive viewport choice. Which is fine for this block, I think, because all it does is provide layout, and a change made on the tablet breakpoint should obviously affect that breakpoint. But in other cases, and this is pondered in WordPress/gutenberg#19909 with a "Responsive editing" toggle, you want to be able to make global edits regardless of viewport chosen. For example, I might want a global font size of 18px as a baseline, and override that to be 14px only on mobile. That's why we need some indication of responsive edits made, and some way to save them for one or more breakpoints, but still probably, by default, edit globally. Make sense? |
@jasmussen Hi! I'm catching up with the proposed solution here — since this is an active issue in WordPress.com and makes many designs/themes look pretty bad on common viewports, it would be important to sort this out in way or the other. It's not just an issue with mobile phone screens, but also regular smaller laptop screens and touch pads. Would it be possible to find an intermediate solution that's not necessarily core first? With caveat that this problem is elsewhere of course, but so far I've understood from folks that it's this issue talked about here? DemosNote how differently the text behaves in editor and frontend, and how broken it looks on smaller screen. (Ignore the broken button color.) 1440px width editor + site (MacBook Pro 13")769px width editor + site ("iPad") |
Thanks for the ping, @simison. I'll take another look and see if we can merge this, or a subset of it, as an interim fix. I can probably recreate the examples you show there myself, but if you have it handy, can you share with me the code you show in the examples? Just ellipsis menu > Copy all content, then paste here, and I'll try and look through all nooks and crannies for what side effect the change might effect. |
@jasmussen Fabulous! I just sent you an invite to a demo site with the template on homepage of that site: https://gutenboarding618965432.wordpress.com/ |
Alright, I took a deep dive. Here's before: That's sub-optimal, to the point of being embarrassing. I think that's worth remembering. Here's after: This is better, but it still has a tradeoff. Right now the breakpoint editing is tied to the preview dropdown. But no different preview is selected, making the icon in the toolbar be misleading. It still shows the desktop icon, even though you are technically editing the tablet breakpoint: This may be acceptable, I don't know. One thing we could do, whether as part of this PR or as a followup, is to revisit the responsive behavior slightly. Right now there's a direct link between the dimensioncontrol in the sidebar, and the preview dropdown — one invokes the other, and vice versa: Outside of this needing some thought upstream, one thing we could do is sever that link, on mobile and tablet breakpoints. So:
This change would make breakpoint-editing with preview a desktop only affair (breakpoint editing on mobile or tablet would be without preview). But we could potentially revisit this as the block editor iframe is maturing and rolling out, perhaps soon to the post editor. That might let us the preview dropdown entirely. If the above is a path forward, the next two steps are:
@johngodley what do you think? |
Writing this as a separate comment to keep things focused. I managed to copy the pattern that is showcased as problematic from the blog you shared, Mikael, thank you. And while it's possible that this PR will mitigate the issues you see, I don't think it will blanket solve the problems. I think there are some problems with the pattern itself, and implicitly, some features we are lacking in the block editor to enable that particular pattern to behave as intended. Here it is: That's a Media & Text block, with an image on the left, and a layout grid block on the right. The layout grid block is there, as far as I can tell, only to control the width of the text, so it's not full-wide unless on the mobile. The problem here is, the Media & Text block has built in responsiveness only for the mobile breakpoint, where it simply stacks in two rows. That means regardless of what responsiveness is built into the layout grid block for the tablet breakpoint, it's not going to be a good tablet breakpoint look: Also, the layout grid, while possible to use without being full width, it really is meant to be the "master container". When it's used inside another block like here, not only will the grid not line up with any other layout grids on the page, but the behavior as mentioned, is likely to be out of sync with different responsive rules. I empathize with the challenge — I get the feeling that Layout Grid was used here solely because it does offer some customizability at responsive breakpoints. This only serves to highlight how much we need responsive editing directly in the block editor, for all blocks. The pattern would be much better if, perhaps, the width of a group could be tweaked on the three breakpoints. If the change mentioned in my previous comment improves things, I'm happy to hear that. But I'm afraid to say that we need additional features in the core editor and core blocks, in order to solve some of the more underlying issues. |
Thanks @jasmussen! We disabled that specific design because indeed multiple issues accumulate making it pretty bad, one of which was Layout Grid problem. I wasn't aware of media+text issues on top of them. @ianstewart @apeatling are you familiar with the media+text block issue Joen mentions and is there issue for it? Should the design be meanwhile constructed using different blocks so that it can be enabled again? |
To be clear, Media & Text is not the problematic part here, its built-in "stack on mobile" responsiveness works fine. The problem is that, outside of a bit of a layout grid pivot, I don't think the layout grid block is ever going to shine when used as an inner-block in this way. It's best used as the root level container for a pattern. So |
Ahh gotcha! So yeah, modifications into the template come to question then. @ianstewart might be good to review other designs for similar structure? |
We could potentially look at some tweaks to the block. Like a per block maximum layout lines setting, that would better accommodate it being used as a child block. |
Responding to Joen's deep dive question, yes that is essentially what this PR does (with some changes needed). It seems an acceptable way to go, although there are edge cases. For example, if you are in desktop mode and resize to a tablet breakpoint then the grid will show the tablet breakpoint, but the core preview dropdown will still show you are on desktop. Maybe not a major problem, I don't know, but a little inconsistent. Would removing the preview feature entirely and letting CSS do its stuff be an option? We could circle back with core enhancements in the future. I'm not the target market for this feature, so I don't really know how useful it is. |
Thank you John, I feel like I owe a milkshake after making you take that deep dive.
The preview feature exists so as to let you make responsive edits in context of a correct viewport. While it's clearly causing some issues with the mobile and tablet breakpoints, honestly to me it works so well on the desktop breakpoint that it would be a pity to remove it entirely. Would it be hard to keep it just for the desktop breakpoint still? |
@aaronrobertshaw I'm currently finishing up the work here and will include it in the next-but-one release of the block (there's one due out soon, so the one after that). |
Supported version of Gutenberg now has this, so we can remove the fallback to reduce clutter
Use Gutenberg viewport matches and resize observer to: - Disable the toolbar preview mode if on mobile - Match the display to the actual viewport, unless on desktop where it matches the Gutenberg preview mode
c507649
to
b1389aa
Compare
I've refactored this PR and clarified some of the behaviour. This is a little different from the above summaries, mainly because Gutenberg allows tablet users to change the preview mode and so the grid should handle this. First a few definitions: The behaviour is now:
Testing
The above conditions should still be true if a desktop browser is resized. |
I tried checking out and compiling this branch: But for whatever reason I don't have the layout grid available to me: It's likely a problem on my end, some process of yarn start / yarn build that I did in wrong succession. Nevertheless I can't test right at this moment, so instead I'll focus on the thorough description. And all of it sounds good, certainly good enough to try out in a version update, then go from there.
If the preview is available from the dropdown menu on tablet, I would tend to think it okay to allow also editing the desktop breakpoint. But I also think it's completely fine to start here, and enable this later if we learn it to be necessary. The primary takeaway from this effort, though, remains that we need better handling of responsive editing in the block editor itself, and that there are lots of learnings from this effort. Thanks again! |
I'm currently having a similar issue. If I checkout master, I can insert a LayoutGrid block. However, following the same process with this PR branch it doesn't look like the block is registered correctly. Saving a post with a Layout Grid block while on master, then loading that post in the editor while on this PR branch, I get a "Your site doesn't include support for thee Layout Grid block" error. Clicking the install button pictured above ends up resulting in an error stating the block is already registered. Trying this independently or along side the Gutenberg plugin doesn't seem to make a difference. I'm not sure what I'm missing either. |
In case it helps. With this PR branch checked out. The block experiments The file is present locally, as well as in Docker. Echoing out the path to that when the script is enqueued gives a valid URL and following that URL does get the JS file. Also tried deleting the |
Sorry, I think VS Code auto-imported some module that I didn't notice, and this confused the WP script dependencies. I've removed this. |
Appreciate the fix @johngodley 👍 This tests well for me as per your instructions when using Chrome or Safari and emulating different devices with them. However, when using the simulator app to test, it still appears as though an iPad tablet displays the desktop grid. I don't have an actual iPad handy at the moment, perhaps others can replicate this or confirm it's actually working? |
@aaronrobertshaw it depends on the actual size of the device. The term 'tablet' is just a placeholder for a width value, and if the actual device has enough pixels to be considered desktop then it will behave as if on desktop (the same would happen for a mobile device that is big enough to be a tablet). Judging by your example it is being detected as desktop because you are seeing the block inspector as a sidebar. On a smaller device the block inspector becomes a full-width overlay. Smaller tablet device: Larger tablet device: |
@johngodley Thanks for the further explanation. I had simply selected the same version of iPad in Chrome and Simulator, expecting they'd restrict to the same dimensions and the resulting display would be the same. When changing the device type in Simulator to other iPads or large iPhones in landscape mode, the editor behaves as described. Sorry for the confusion. |
Just to confirm, do you mean it's working as expected in my description above? If so, is this good to go? If not, can tweaks to the breakpoint values be done in a future PR so the more important fixes here can go out? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests well for me 🚢
- Grid displays correctly on desktop, tablet and mobile
- The toolbar preview button was only present when on desktop or tablet
- Inspector breakpoint buttons changed the preview device when on desktop or tablet
- Inspector breakpoint buttons did not change the preview when on mobile
- Grid was correctly set to current device or lower device when preview mode had been changed.
@roo2 Given the benefits of this PR, is it best to land it and improve further in a separate PR? |
I agree listed tradeoffs sound fine for now given the improvements, and can
be worked out in separate PRs. Majority of customers modify pages on
desktop resolution.
…On Thu, 4 Feb 2021, 10:52 Joen A., ***@***.***> wrote:
@roo2 <https://github.com/roo2> Given the benefits of this PR, is it best
to land it and improve further in a separate PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#161 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAVJABK5HW7KYA4DCQXH7LS5JN3ZANCNFSM4U5KQZ4Q>
.
|
Well spotted. I've fixed this.
It does keep it properly, but what you highlighted is the keyboard focus, not the currently selected item. I've changed the dropdown menu to show a checkbox now: The blue box will still be where the keyboard is focussed, but at least you can now tell which one is selected. I'll merge this as-is. It'll still need a review on wp.com and these new tweaks can be tested there. If further changes are needed they can be split into individual PRs. If all goes well then I will try and ship this (wp.com and wp.org) early next week as version 1.5. Thanks everyone! |
Currently, when opening the editor on mobile, the layout grid is rendered in desktop mode.
its display is controlled by the
__experimentalGetPreviewDeviceType
property which can be set via gutenberg's preview mode, or through the block settings for the layout grid block.__experimentalGetPreviewDeviceType
always defaults to desktop, so the layout grid always attmepts to render with maximum columns in the editor. This leads to "broken" looking pages in the mobile editor.Context
Initial report
Automattic/wp-calypso#42148
More discussion and investigation
#152
Code changes and proposal
Goals
My aim was to keep the "Preview Mode" functionality working as it is while also supporting editing on mobile intuitively.
Approach
I split the device detection into the "preview mode" represented by
selectedPreviewDeviceType
and the "rendering mode" represented byrenderDeviceType
. The preview mode remains based on the__experimentalGetPreviewDeviceType
flag fromcore/block-editor
while the render mode is based solely on the width of the "visual editor".When the preview mode is changed, or the window is resized, the rendering mode is recomputed. The result is quite intuitive I think!
Test instructions