Edit Post: Fix sidebar scroll overflow. #18810
Conversation
epiqueras
added
[Type] Bug
epiqueras
requested review from
ajitbohra,
chrisvanpatten,
jorgefilipecosta,
talldan and
youknowriad
as code owners
packages/edit-post/src/components/sidebar/sidebar-header/style.scss
Outdated
Show resolved
Hide resolved
| @@ -54,6 +54,7 @@ | |||
| bottom: 0; | |||
| left: 0; | |||
| background: $white; | |||
| overflow: auto; | |||
There was a problem hiding this comment.
Ohhh, now I get it now!
While this seems to fix the issue if you test on the browser, there's a weird issue in Safari iOS (specially old versions) where the only scrollable element should be the html element otherwise the sticky behavior doesn't work properly.
I think the fix should be to make sure the html height increases with the sidebar allowing it to scroll. This also might make the z-index fix for the sidebar header unecessary.
Anyway this is a complex topic that only @jasmussen understands properly :P so I'd prefer to wait for his feedback. He also has this PR open that I think is related #18686
|
cc @jasmussen |
|
How embarrassing that I missed the ping the first time, my apologies, and thank you for the re-sping. Always feel free to send me a reminder on stuff like this, it's much appreciated. I took a look now, and I think we need a good solid rebase before we can make a final decision, because since this PR, #18686 has been merged which forces the toolbar to the top on mobile. Some quick tests, though. Master on mobile:
Master on desktop:
Master on ios:
After:
As far as I can see, the problem this PR intended to solve is no longer an issue, but I may be missing some nuance. I recall Riad and I having some confusion about the decision to always show the tabs sticky fixed at the top of the sidebar, this PR does appear to make that more consistent, which if it has value could be the purpose of the PR. But we can probably revisit after the rebase. |
epiqueras
force-pushed
the
fix/sidebar-scroll-overflow
branch
from
a2232d2 to
d009bb1
Compare
|
No worries. Yeah, it looks like most of the issues were fixed. I do think there is some value in the sticky header consistency. This PR still fixes that after the rebase, but there might be a better way of doing it now. |
|
I have no strong opinion one way or the other whether the header should be fixed. Code seems alright to me. In #18667 a new UI is being explored, but I suspect the tabs might benefit similar to how these might. Have you tested this in Edge or Windows? It's okay to disable the sticky for those browsers, it just can't be unusable. |
Don't you find it beneficial to be able to switch between tabs without having to scroll back up? |
It never bothered me, but I also wouldn't mind it being reapplied! 👍 👍 from me. |
|
@mapk @youknowriad @aduth Thoughts? |
1 similar comment
|
@mapk @youknowriad @aduth Thoughts? |
|
Throw in a fix for the recent Firefox issue and I'll approve this one 😅 |
|
What is the issue? |
|
This one: #20206 |
|
That seems unrelated. |
|
Looks like this was fixed elsewhere. |
Description
A combination of #17926 and #18044 broke the block inspector controls sidebar scrolling behavior. The sidebar content covered the header when scrolling and on mobile it did not scroll at all.
This PR fixes the issues.
How to test this?
Check that scrolling the sidebar works as expected on all screen sizes and that the header sticks to the top like it should.
Types of Changes
Bug Fix: Fix sidebar scroll overflow issues.
Checklist: