Ensure device previews extra scrollbar only appears when needed #62952
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
talldan
added
[Type] Bug
|
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. |
talldan
force-pushed
the
fix/device-previews-scrollbar-behavior
branch
from
277e886 to
877561b
Compare
|
Size Change: +12 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
I’m glad you noticed too. I tested this, it does what it says on the tin and I would count it as an improvement. Yet another thing that I was considering to be part of the issue is that the device previews put a margin around the iframe that presumably should be visible on all sides (as it is in WP 6.5). With this branch the bottom margin isn’t included in the overflow. Here’s 6.5: device-preview-6.5.movHere’s this branch: device-preview-62952.movSo this isn’t quite there if we’d like to match 6.5. I already had a branch for that but so far it’s considerably less simple. Beyond that, there are some design considerations that perhaps should come into play but they may lead to a intentional departure from how device previews shipped in 6.5 and therefore maybe have to wait because, as I understand it, this late in the 6.6 release cycle it’s not supposed to be done. Yet I'll expound a bit on that anyway. First off, that bottom margin doesn’t seem very valuable. It’s also inconsistent with how "focus" mode does a similar thing but doesn’t apply it to the bottom (and does it with Then another consideration is that horizontal overflow is actually prevented in device previews by resizing the iframe horizontally. I wonder if it wouldn’t be a better to do the same vertically. That way a bottom margin would always be visible and there’d be no reason for a second scrollbar. I noticed that this branch can be made that way with just one more line of CSS. I’m including a demo to illustrate despite that it may be outside of what we can consider for this branch. device-preview-no-overflow-y.mov |
stokesman
approved these changes
Jun 29, 2024
There was a problem hiding this comment.
Despite how I pointed out this doesn’t quite restore how device previews were in 6.5 it’s still a very simple fix for part of it and I think it’d be worthwhile. It didn’t cause any unintended effects that I could find. I tested zoom out mode and it seems unaffected.
I did tinker more on restoring the device previews just as they were in 6.5 and #62989 is the most minimal way I found. I left it drafted because I'm calling it a night and probably should add a bit more detail to explain some some changes.
|
Thanks @stokesman. Lets get this one in for 6.6, and then follow up with a more considered fix for 6.7. 👍 |
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Jul 1, 2024
Co-authored-by: talldan <[email protected]> Co-authored-by: stokesman <[email protected]>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.6 branch to get it included in the next release: 555a64e |
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
…Press#62952) Co-authored-by: talldan <[email protected]> Co-authored-by: stokesman <[email protected]>
Just wanted to note that after #63724, the margin around device previews is fully scrollable as it was in 6.5 🎉 |
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?
A small improvement to go alongside #62940.
I noticed even though that PR removes the third scrollbar, there are still often two even when the device fully fits in the browser window, and scrolling the second one doesn't do anything useful.
Why?
The issue seems to be with the
.block-editor-iframe__scale-container, which always extends beyond the browser viewport when Mobile/Tablet view is active.How?
I'm no css expert, but adding
display: flexis an easy way to make it fit in the screen, with no obvious repercussions.Testing Instructions
Also do a general smoke test of things like:
Screenshots or screencast