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

Fix sidebar on mobile. #18937

Merged
merged 1 commit into from Dec 6, 2019
Merged

Fix sidebar on mobile. #18937

merged 1 commit into from Dec 6, 2019

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Dec 5, 2019

Scrolling is broken in the sidebar on mobile. This is likely related to the recent refactor, and because some delicate behavior is there. This PR fixes it:

sidebar

So, what happened? I'm not entirely sure, and therefore it would be good to get lots of testing on this. It tests well for me, but still.

The behavior, best as I can tell, that we want is for the modal sidebar to scroll, but the document title and close button should stay fixed at the top, as should the two tabs.

@jasmussen jasmussen added the [Type] Bug label Dec 5, 2019
@jasmussen jasmussen requested a review from youknowriad Dec 5, 2019
@jasmussen jasmussen requested a review from talldan as a code owner Dec 5, 2019
@jasmussen jasmussen self-assigned this Dec 5, 2019
@@ -3,8 +3,6 @@
padding-left: 0;
padding-right: $grid-size-small;
border-top: 0;
position: sticky;
top: 0;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 5, 2019

Contributor

Capture d’écran 2019-12-05 à 2 44 11 PM

This is not sticky on desktop now. I wonder if it's expected and if it's related to this change here.

This comment has been minimized.

Copy link
@draganescu

draganescu Dec 5, 2019

Contributor

I have tested and even if unintended this change is good b/c the sticky behaviour of tabs in the sidebar on desktop was a bad peek-a-boo effect.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Dec 5, 2019

Author Contributor

Interesting. Yeah as Andrei suggests, this is what I see:

current behavior

This is definitely a separate regression, worth also looking at here. At a glance it looks like a z-index issue.

Before I go and fix it, I wonder why we made those sticky in the first place, I can't recall the circumstances, perhaps because I was up early today :D

Copy link
Contributor

draganescu left a comment

on master currently you cannot scroll at all, while this enables scrolling again.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Dec 6, 2019

Merging this. Now there are three of us that are aware of how the tabs used to be sticky — if anyone worries about it, we'll know where to fix it.

@jasmussen jasmussen merged commit 4310db7 into master Dec 6, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jasmussen jasmussen deleted the fix/sidebar branch Dec 6, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
scruffian added a commit to scruffian/gutenberg that referenced this pull request Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.