[Gatsby Docs Update] Fixed iOS overscroll issues, and redesigned mobile sidebar button to … #10761

joecritch · 2017-09-20T17:48:37Z

Added some fixes from http://blog.christoffer.online/2015-06-10-six-things-i-learnt-about-ios-rubberband-overflow-scrolling/ for the overflow scrolling. Seems to work.
Subtle sidebar transition tweaks
Redesigned mobile sidebar toggle. Previously there was an issue where you'd have to tap it twice, as only Safari's toolbar would appear on the first tap. . This caused a bit of yak shaving on the redesign, but I think it works just as well, if not better.

@bvaughn

reactjs-bot · 2017-09-20T17:53:05Z

Deploy preview ready!

Built with commit 9beb61f

https://deploy-preview-10761--reactjs.netlify.com

bvaughn

In some ways, I like the floating button style!

However it does have the downside of truncating titles that are longer than a word or two, (which includes a lot of titles).

The new touch logic also makes the Android experience worse. The first scroll attempt (up or down) gets blocked. The second one works. But then trying to scroll in the opposite direction is blocked on the first try also. This doesn't seem to happen on the main gatsby branch.

I don't actually have an iPhone charged to test what you're fixing at the moment, but I'll plug mine in now so I can try it out after lunch.

bvaughn · 2017-09-20T17:55:17Z

www/src/components/StickyResponsiveSidebar/StickyResponsiveSidebar.js

+    document.addEventListener('touchmove', this.handleDocumentTouchMove);
+    document
+      .querySelector('.scrollable')
+      .addEventListener('touchstart', this.handleSidebarTouchStart);


Strong initial dislike to all of the global handlers/selectors here. If we have to do this, I think we should wrap it up in a separate component rather than mix it into the sticky component.

Ideally we could do this in a more "React" way (with props/refs) but I'd have to think about that a bit more. Might be harder to do.

Edit:
I see you did add a comment and link below - still might be good to comment here too.

It might be possible to make it a bit more 'React' ish. For now we could start by adding a comment and linking to the blog post that describes this fix for the iOS overscroll bug. (http://blog.christoffer.online/2015-06-10-six-things-i-learnt-about-ios-rubberband-overflow-scrolling/)

bvaughn · 2017-09-20T17:55:44Z

www/src/components/LayoutHeader/Header.js

+
+css.global('.is-menu-open #header', {
+  transform: 'translateY(-100%)',
+});


It would be nice to avoid global styles for this. I suggest we pass an isOpen prop do this component instead and (probably) expose a toggle function to the sticky sidebar via context. (I'll be happy to help implement this change.)

bvaughn · 2017-09-20T18:00:11Z

www/src/components/Flex/Flex.js

@@ -25,11 +25,13 @@ const Flex = ({
  shrink = 1,
  type = 'div',
  valign = 'flex-start',
+  className,


nit: Alpha-sort props 😄

flarnie

I agree with Brian's suggestions about making things less global. That could be done in a follow-up, and/or Brian or myself could pick it up, shouldn't block merging this imo.

Thanks for tracking down a fix for this iOS issue!

I haven't manually tested this with Android, but can we figure out and try to fix the issue Brian mentioned with scrolling there? I would merge this if Android and other browsers are all functional with this change.

joecritch · 2017-09-20T19:42:50Z

@bvaughn @flarnie

Fixed the scrolling issue. It was actually the overflow-x: hidden on the article. I've fixed the horizontal scrolling with a better solution.
Agreed that the text truncation is a trade-off with the floating button. However, you do already have context of what page you're on, and I feel like it's better than a full-width button when being 44px further up.
Yeah, the global event handlers and CSS aren't ideal. Don't have the time to improve these tonight, so any help would be great.

Mathspy · 2017-09-21T01:36:21Z

For the lack of a better place and for the sake of not to opening my first React PR to add a line:

This tiny little visual bug (the unneeded scroll bar) exists on Chrome 61, and it should be fixed with a little overflow-y: hidden

Loving the new website so far ❤️ ❤️ ❤️ Amazing work everyone!

joecritch · 2017-09-21T09:58:37Z

@Mathspy Thank you! It's on the list 👍

…accommodate Safari inert 44px

joecritch · 2017-09-21T16:36:19Z

www/src/templates/docs.js

@@ -30,7 +30,7 @@ const sectionList = sectionListA
    }),
  );

-const Docs = ({data, location}) => (
+const Docs = ({data, location}) => console.log('render Docs') || (


this doesn't fire when you resize the window (above xsmall)

flarnie

lgtm - I manually tested Firefox at various widths. Going to manually test Android, iOS, Chrome 61, and IE, but I think we can tackle any bugs with those as follow-up or separate work.

This is looking so slick!

flarnie · 2017-09-22T00:08:52Z

Just manually tested on Android 7.0, Samsung Galaxy S6. Looks and feels pretty good!

It seems like there is sometimes a lag where I can't scroll for a moment right after the page loads, but hard to say for sure. Still seems usable to me.

bvaughn · 2017-09-22T14:37:09Z

It seems like there is sometimes a lag where I can't scroll for a moment right after the page loads, but hard to say for sure. Still seems usable to me.

Does this happen on master too, by chance?

bvaughn · 2017-09-22T14:57:22Z

Okay! Taking another look at this 😁

Trying what I think is the latest build of this (59c43b440752d04852a6a86c) on my Android 8.0 phone:

The nav doesn't open when touched. Instead a Chrome pop expands from the bottom to show a link to more info about the tapped text (eg "Hello World"). If I'm careful and click on the arrows instead, this doesn't happen, but neither does the menu open. (Edit: Perhaps this could be fixable my styling this text to be non-selectable? eg user-select: none).
The text truncation still happens too. eg on the Tutorial page, the nav just shows "Tutorial: Int...".
The scroll-stuck behavior still blocks the first attempt to scroll in a new direction (as described previously).

I don't see any of the behavior described above on my device on master.

I can also reproduce 2 and 3 on the Android simulator (but not 1, perhaps b'c the mouse is a different kind of input device...not sure).

flarnie

Changing my review since Brian found Android 8.0 still has issues. That's frustrating. Maybe we can talk more in chat about which versions of Android we want to test and how to get them all tested/working before launch.

flarnie · 2017-09-22T23:51:36Z

Ok - so I wonder if the difference between Brian and my test is the version of mobile Chrome we are running?

Chrome 61 just came out, and had some breaking changes related to scrolling. We just fixed a bug in Draft.js related to this.

@bvaughn what version of mobile chrome do you have on your Android phone?

Mine is 60.0.3112.116.

props to @sophiebits for talking through this and giving me the idea.

joecritch · 2017-09-23T18:29:26Z

Thanks for the reviews, folks. I'm closing this, as 1/ a rebase I did on this branch caused a regression in the scrolling, and 2/ it'll be easier if they're divided up into separate PR's anyway. 👍

facebook-github-bot added the CLA Signed label Sep 20, 2017

bvaughn reviewed Sep 20, 2017

View reviewed changes

flarnie suggested changes Sep 20, 2017

View reviewed changes

joecritch changed the title ~~Fixed iOS overscroll issues, and redesigned mobile sidebar button to …~~ [Gatsby Docs Update] Fixed iOS overscroll issues, and redesigned mobile sidebar button to … Sep 21, 2017

joecritch added 2 commits September 21, 2017 17:15

Fixed iOS overscroll issues, and redesigned mobile sidebar button to …

e197968

…accommodate Safari inert 44px

fixed scrolling issue on xsmall

87cc5c5

joecritch force-pushed the gatsbyDocsSidebarIOSSafariFixes branch from 0fb9154 to 87cc5c5 Compare September 21, 2017 16:16

joecritch added 2 commits September 21, 2017 17:17

moved <Flex /> className prop

cdddfdb

wip - refactoring to use this.context

8c0555c

joecritch commented Sep 21, 2017

View reviewed changes

joecritch added 3 commits September 21, 2017 20:05

prettier

9cb25bf

revert context work and change icon colour back to white

d221b42

iPhone X fix

9beb61f

flarnie approved these changes Sep 21, 2017

View reviewed changes

flarnie suggested changes Sep 22, 2017

View reviewed changes

joecritch closed this Sep 23, 2017

This was referenced May 20, 2020

[Snyk] Security upgrade lighthouse from 2.0.0 to 6.0.0 cajonemsft/react#22

Open

[Snyk] Security upgrade lighthouse from 2.0.0 to 6.0.0 heshamelmasry77/react#11

Open

This was referenced Mar 23, 2022

[Snyk] Fix for 1 vulnerabilities cajonemsft/react#96

Open

[Snyk] Fix for 1 vulnerabilities heshamelmasry77/react#66

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Gatsby Docs Update] Fixed iOS overscroll issues, and redesigned mobile sidebar button to … #10761

[Gatsby Docs Update] Fixed iOS overscroll issues, and redesigned mobile sidebar button to … #10761

joecritch commented Sep 20, 2017 •

edited

Loading

reactjs-bot commented Sep 20, 2017 •

edited

Loading

bvaughn left a comment •

edited

Loading

bvaughn Sep 20, 2017 •

edited

Loading

flarnie Sep 20, 2017 •

edited

Loading

bvaughn Sep 20, 2017 •

edited

Loading

bvaughn Sep 20, 2017

flarnie left a comment

joecritch commented Sep 20, 2017 •

edited

Loading

Mathspy commented Sep 21, 2017

joecritch commented Sep 21, 2017

joecritch Sep 21, 2017

flarnie left a comment

flarnie commented Sep 22, 2017

bvaughn commented Sep 22, 2017

bvaughn commented Sep 22, 2017 •

edited

Loading

flarnie left a comment

flarnie commented Sep 22, 2017

joecritch commented Sep 23, 2017

[Gatsby Docs Update] Fixed iOS overscroll issues, and redesigned mobile sidebar button to … #10761

[Gatsby Docs Update] Fixed iOS overscroll issues, and redesigned mobile sidebar button to … #10761

Conversation

joecritch commented Sep 20, 2017 • edited Loading

reactjs-bot commented Sep 20, 2017 • edited Loading

bvaughn left a comment • edited Loading

Choose a reason for hiding this comment

bvaughn Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

flarnie Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

bvaughn Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

bvaughn Sep 20, 2017

Choose a reason for hiding this comment

flarnie left a comment

Choose a reason for hiding this comment

joecritch commented Sep 20, 2017 • edited Loading

Mathspy commented Sep 21, 2017

joecritch commented Sep 21, 2017

joecritch Sep 21, 2017

Choose a reason for hiding this comment

flarnie left a comment

Choose a reason for hiding this comment

flarnie commented Sep 22, 2017

bvaughn commented Sep 22, 2017

bvaughn commented Sep 22, 2017 • edited Loading

flarnie left a comment

Choose a reason for hiding this comment

flarnie commented Sep 22, 2017

joecritch commented Sep 23, 2017

joecritch commented Sep 20, 2017 •

edited

Loading

reactjs-bot commented Sep 20, 2017 •

edited

Loading

bvaughn left a comment •

edited

Loading

bvaughn Sep 20, 2017 •

edited

Loading

flarnie Sep 20, 2017 •

edited

Loading

bvaughn Sep 20, 2017 •

edited

Loading

joecritch commented Sep 20, 2017 •

edited

Loading

bvaughn commented Sep 22, 2017 •

edited

Loading