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

Improve Docs navigation on handheld devices. #7640

Closed
wants to merge 1 commit into from
Closed

Improve Docs navigation on handheld devices. #7640

wants to merge 1 commit into from

Conversation

Andreyco
Copy link
Contributor

Fixes #7519

Summary:
JS detects handheld device by sniffing UA string (very primitive detection). If on handheld device, event listener is registered. Event handler toggles Docs Navigation overlay after clicking on "Docs" nav button.
Original Docs Navigation panel is taken out of the natural page flow using pure CSS and is styled to look "good" on device. As a result of this, Navigation overlay is ONLY visible when you are at Docs page, otherwise "Docs" nav button takes you Docs page first.

iPhone/iPad previews
iphone
ipad

Summary:
JS detects handheld device by sniffing UA string (very primitive detection). If on handheld device, event listener is registered. Event handler toggles Docs Navigation overlay after clicking on "Docs" nav button.
Original Docs Navigation panel is taken out of the natural page flow using pure CSS and is styled to look "good" on device.  As a result of this, Navigation overlay is ONLY visible when you are at Docs page, otherwise "Docs" nav button takes you Docs page first.
@ghost
Copy link

ghost commented May 19, 2016

By analyzing the blame information on this pull request, we identified @vjeux and @janicduplessis to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels May 19, 2016
@Andreyco
Copy link
Contributor Author

Also, I am thinking about additional changes.
1. Further improvement
See the picture, current state on the left, proposed state on the right
image

Right now, nav bar is position: fixed to the top. Part of Scrollable area is underneath, which results in problem - nav bar of unknown height can overflow content underlying scrollable area and it's content. This does happen on small screens, where nav bar is 2 line tall. Padding are used to fix this but as we know this is error prone.

Instead, I propose to make nav bar position: static and use flex to make fill the rest of viewport height and make that area scrollable. Flex is used already, we should not avoid it here.

2. Scrollable sidebar on desktop
There are possible two scenarios:
a, tall content vs short sidebar
b, short content vs tall sidebar

Either of these results in unnecessary scrolling. Making sidebar scrollable will make it independent from page scroll position. Will need to play with it, try out scrolling active link to viewport on page load, ...

Number 2 is possible standalone PR

@Andreyco
Copy link
Contributor Author

cc @vjeux


// Primitive mobile detection
function isMobile() {
return ( /Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(navigator.userAgent) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test the same media query that we use in the css to figure out if we should display the mobile version? Or are there cases like on tablet where you want two different versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I answered my question, it is for tablet. Looks good

@vjeux
Copy link
Contributor

vjeux commented May 19, 2016

I fear that people may no know how to close that menu. Do you think we should add a X icon at the top right?

@vjeux
Copy link
Contributor

vjeux commented May 19, 2016

I'm down for your suggestion #2 as well. Flexbox is now well supported (except for ie8-9 but I hope developers don't use those...). As you mentioned, it would indeed be better in a different pull request

@vjeux
Copy link
Contributor

vjeux commented May 19, 2016

@facebook-github-bot shipit

@vjeux
Copy link
Contributor

vjeux commented May 19, 2016

Thanks a lot!

@ghost ghost added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels May 19, 2016
@ghost
Copy link

ghost commented May 19, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@Andreyco
Copy link
Contributor Author

Can you test the same media query that we use in the css to figure out if we should display the mobile version? Or are there cases like on tablet where you want two different versions?

I certainly could use device screen width (similar to max-device-width used in CSS) and make decision (not) to attach event handler and drop UA sniffing.

I fear that people may no know how to close that menu. Do you think we should add a X icon at the top right?

You are right, I will add close button soon. Also, are you down for proposed change #1 - to make nav bar static If you are, I can deliver both in another PR

I'm down for your suggestion #2 as well. Flexbox is now well supported (except for ie8-9 but I hope developers don't use those...). As you mentioned, it would indeed be better in a different pull request

Great, will work on this...

@ghost ghost closed this in 00c7780 May 20, 2016
zebulgar pushed a commit to nightingale/react-native that referenced this pull request Jun 18, 2016
Summary:
Fixes facebook#7519

JS detects handheld device by sniffing UA string (very primitive detection). If on handheld device, event listener is registered. Event handler toggles Docs Navigation overlay after clicking on "Docs" nav button.
Original Docs Navigation panel is taken out of the natural page flow using pure CSS and is styled to look "good" on device.  As a result of this, Navigation overlay is ONLY visible when you are at Docs page, otherwise "Docs" nav button takes you Docs page first.

iPhone/iPad previews
![iphone](https://cloud.githubusercontent.com/assets/829963/15409630/f1a64b1a-1e15-11e6-92eb-f85c5cd06754.gif)
![ipad](https://cloud.githubusercontent.com/assets/829963/15409631/f1a6f952-1e15-11e6-8f5c-6f89f54e6814.gif)
Closes facebook#7640

Differential Revision: D3325440

Pulled By: vjeux

fbshipit-source-id: a06b21d743d56bfea5db5b750836856c3af9bbe2
samerce pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
Fixes facebook#7519

JS detects handheld device by sniffing UA string (very primitive detection). If on handheld device, event listener is registered. Event handler toggles Docs Navigation overlay after clicking on "Docs" nav button.
Original Docs Navigation panel is taken out of the natural page flow using pure CSS and is styled to look "good" on device.  As a result of this, Navigation overlay is ONLY visible when you are at Docs page, otherwise "Docs" nav button takes you Docs page first.

iPhone/iPad previews
![iphone](https://cloud.githubusercontent.com/assets/829963/15409630/f1a64b1a-1e15-11e6-92eb-f85c5cd06754.gif)
![ipad](https://cloud.githubusercontent.com/assets/829963/15409631/f1a6f952-1e15-11e6-8f5c-6f89f54e6814.gif)
Closes facebook#7640

Differential Revision: D3325440

Pulled By: vjeux

fbshipit-source-id: a06b21d743d56bfea5db5b750836856c3af9bbe2
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
Fixes facebook#7519

JS detects handheld device by sniffing UA string (very primitive detection). If on handheld device, event listener is registered. Event handler toggles Docs Navigation overlay after clicking on "Docs" nav button.
Original Docs Navigation panel is taken out of the natural page flow using pure CSS and is styled to look "good" on device.  As a result of this, Navigation overlay is ONLY visible when you are at Docs page, otherwise "Docs" nav button takes you Docs page first.

iPhone/iPad previews
![iphone](https://cloud.githubusercontent.com/assets/829963/15409630/f1a64b1a-1e15-11e6-92eb-f85c5cd06754.gif)
![ipad](https://cloud.githubusercontent.com/assets/829963/15409631/f1a6f952-1e15-11e6-8f5c-6f89f54e6814.gif)
Closes facebook#7640

Differential Revision: D3325440

Pulled By: vjeux

fbshipit-source-id: a06b21d743d56bfea5db5b750836856c3af9bbe2
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants