-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Try: Show metaboxes peeking in even on tiny screens. #20262
Conversation
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.
LGTM 👍
@@ -27,7 +27,7 @@ | |||
|
|||
.edit-post-visual-editor .block-editor-writing-flow__click-redirect { | |||
// Allow the page to be scrolled with the last block in the middle. |
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.
we should fix end to end tests before merging
Thanks, that would explain why the tests keep failing. It looks like this test is failling:
I don't understand why that would fail based on this change. I tried more or less manually repeating the test and I see no scrolling happening. How small is the viewport supposed to be? |
@@ -27,7 +27,7 @@ | |||
|
|||
.edit-post-visual-editor .block-editor-writing-flow__click-redirect { | |||
// Allow the page to be scrolled with the last block in the middle. | |||
min-height: 50vh; | |||
min-height: 40vh; |
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 feels like a random number used to give some space for meta boxes. I'm not sure if we can do better though.
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.
We can, but not without a biggish refactor of the component. There's an argument that it was a bit random before as well — 50vh is half of the entire viewport. But it should have been half of the viewport, minus header, footer and optionally the adminbar if not fullscreen, with extra rules for mobile contexts.
92086b6
to
92e94e7
Compare
I'm not exactly sure why the test was failing (locally it passes). I'm trying a "guess-fix" in the last commit. Let's see how it goes. |
Size Change: 0 B Total Size: 864 kB ℹ️ View Unchanged
|
In the past, some very viewport dimension sensitive tests failed due to things like text wrapping in the test VM, but not when we test it manually. That could come down to something as simple as the Linux VM running a different/larger font than us Mac users. |
Tests pass! Thumbs up? |
Go |
:D I need an approval! |
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.
:)
* Try: Show metaboxes peeking in even on tiny screens. Fixes #19709. * Try fixing the e2e test Co-authored-by: Riad Benguella <benguella@gmail.com>
* Try: Show metaboxes peeking in even on tiny screens. Fixes #19709. * Try fixing the e2e test Co-authored-by: Riad Benguella <benguella@gmail.com>
Fixes #19709.
19709 reports that if metaboxes are present on the page, on a new page they might be out of sight below the fold and therefore hard to find.
The CSS to enable the editor to scale as it does is rather complicated and could use a greater refactor. For example, the div that is dedicated to
editor-styles-wrapper
is currently born with a hard coded top padding, which seems like something the theme should provide. Combine that with various flex rules, and there's an opportunity to make strides here.But the canvas is currently in the midst of a great deal of open heart surgery with DOM nodes being removed left and right, and lots of other block paddings being revisited in #18667 it really doesn't feel like the right time to rearrange those bits.
In that vein, this PR does the smallest possible thing it can to fix the issue, which is to simply reduce the min-height of the click redirect.
If you make the viewport less tall than this, the left navigation sidebar will start to scroll.