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

doc: fix scrolling on iOS devices #5878

Closed
wants to merge 3 commits into from
Closed

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Mar 23, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?

Affected core subsystem(s)

doc

Description of change

Fixes an issue that prevented scrolling from going past large code blocks on iOS devices.

Fixes: #5861

@@ -1,5 +1,6 @@
/*--------------------- Layout and Typography ----------------------------*/
html {
-webkit-overflow-scrolling: touch;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enables momentum scrolling. The scrolling experience with it on is a lot better but its use is discouraged.

I've had no issues while testing but we can remove it if we think it will cause problems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familar with the way IOS scrolls, but do I understand correctly that momentum scrolling is disabled by default on web pages and enabled in apps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's correct web pages, by default, don't have the same inertia apps have.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think this may have negative effects on macs? we should probably restrict it to some size? (wish there was a better way 😞 )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like what? I've tried it on my mac, and it doesn't seem to affect any of the browsers I've tested it on (Safari, Chrome, Firefox).

@mscdex mscdex added the doc Issues and PRs related to the documentations. label Mar 23, 2016
@silverwind
Copy link
Contributor

LGTM besides the momentum scrolling, which I have no opinion on.

@benjamingr
Copy link
Member

Probably dumb question - is there any way I could test this without pulling the branch and building the docs?

The fix looks fine but I want to actually test it on my phone and that's problematic with the networking mechanics on this machine.

@lpinca
Copy link
Member Author

lpinca commented Mar 23, 2016

@benjamingr You can connect your iOS device to a mac and use Safari dev tools from that mac, don't know if this helps.

@@ -281,7 +282,8 @@ pre {
vertical-align: top;
background: #f2f2f2;
margin: 1em;
overflow-x: auto;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using white-space: pre-wrap; now so this should be no longer required.

@silverwind
Copy link
Contributor

Still LGTM. Any more opinions regarding momentum scrolling?

@lpinca
Copy link
Member Author

lpinca commented Mar 29, 2016

@silverwind I've found a new way to fix the issue. We can just reset the overflow property on #content and #column1.interior in the media query.

This also makes the scroll works like every other web page so we don't have to enable momentum scrolling.

Question: isn't this stuff redundant? It's already included in the previous case.

@silverwind
Copy link
Contributor

Question: isn't this stuff redundant? It's already included in the previous case.

Yeah, definitely redundant. Maybe the (max-width: 1024px) and (orientation: portrait) was meant to be a (max-height: 1024px) and (orientation: portrait), but don't think such a rule would make much sense either way. Probably best to drop it.

Will check out your other changes soon.

@silverwind
Copy link
Contributor

LGTM

@lpinca
Copy link
Member Author

lpinca commented Mar 31, 2016

I've rebased against master and squashed commits into a single one.

Fixes an issue that prevented scrolling from going past large code
blocks on iOS devices.

Fixes: nodejs#5861
PR-URL: nodejs#5878
@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

LGTM if @nodejs/documentation folks are happy.

@eljefedelrodeodeljefe
Copy link
Contributor

LGTM

@silverwind
Copy link
Contributor

@lpinca something about this recent change makes text very tiny on Android Chrome, it looks like Chrome switches to non-mobile mode now.

I'm thinking we should maybe specify

<meta name="viewport" content="width=device-width, initial-scale=1">

and then remove the font-size rules in the media queries (the default 1.8em on #content looks acceptable on my Nexus with the viewport tag, everything above seems to big).

@lpinca
Copy link
Member Author

lpinca commented Apr 3, 2016

@silverwind like this?

@lpinca
Copy link
Member Author

lpinca commented Apr 3, 2016

Hmm doesn't work on the iPhone with the latest commit.

@silverwind
Copy link
Contributor

Works for me on Android at least. Oh, and I noticed another minor issue with unbalanced margins on those <pre>s. I'd suggest removing this rule:

@media only screen and (max-width: 1024px) { pre { margin-right: 0; } }

@lpinca
Copy link
Member Author

lpinca commented Apr 4, 2016

We can't really use

<meta name="viewport" content="width=device-width, initial-scale=1">

as it will reduce the screen size by half on retina displays.
Can't we just tweak the font size?

I'd suggest removing this rule:

@media only screen and (max-width: 1024px) { pre { margin-right: 0; } }

+1 for the margin fix.

@lpinca
Copy link
Member Author

lpinca commented Apr 4, 2016

I've removed the viewport meta tag and increased the font size in the media queries. It works fine on my iPhone. Waiting for feedbacks.

@silverwind
Copy link
Contributor

Looking good on Android too. LGTM!

@silverwind
Copy link
Contributor

Thanks! Landed in d2577de.

silverwind pushed a commit that referenced this pull request Apr 9, 2016
Fixes an issue that prevented scrolling from going past large code
blocks on iOS devices. Also fixes a few minor styling issues that
came up in the discussion.

Fixes: #5861
PR-URL: #5878
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Lindstädt <[email protected]>
@silverwind silverwind closed this Apr 9, 2016
@lpinca lpinca deleted the gh-5861 branch April 9, 2016 16:24
MylesBorins pushed a commit that referenced this pull request Apr 11, 2016
Fixes an issue that prevented scrolling from going past large code
blocks on iOS devices. Also fixes a few minor styling issues that
came up in the discussion.

Fixes: #5861
PR-URL: #5878
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Lindstädt <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
Fixes an issue that prevented scrolling from going past large code
blocks on iOS devices. Also fixes a few minor styling issues that
came up in the discussion.

Fixes: #5861
PR-URL: #5878
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Lindstädt <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Fixes an issue that prevented scrolling from going past large code
blocks on iOS devices. Also fixes a few minor styling issues that
came up in the discussion.

Fixes: #5861
PR-URL: #5878
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Lindstädt <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 20, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Fixes an issue that prevented scrolling from going past large code
blocks on iOS devices. Also fixes a few minor styling issues that
came up in the discussion.

Fixes: #5861
PR-URL: #5878
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Lindstädt <[email protected]>
This was referenced Apr 21, 2016
jasnell pushed a commit that referenced this pull request Apr 26, 2016
Fixes an issue that prevented scrolling from going past large code
blocks on iOS devices. Also fixes a few minor styling issues that
came up in the discussion.

Fixes: #5861
PR-URL: #5878
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Robert Lindstädt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants