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

feat(v2): hide navbar on scroll #2055

Merged
merged 4 commits into from
Dec 12, 2019
Merged

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Nov 25, 2019

Motivation

I propose to consider including in my opinion a good feature called "Auto-Hide Sticky Header". The bottom line is simple - navbar hides when scrolling down, and is shown again when scrolling up. This helps the user to better focus on the content. Since not everyone may like it, this option is enabled in the theme config.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

The hideOnScroll option must be enabled.

  themeConfig: {
    navbar: {
      hideOnScroll: true,

AwesomeScreenshot-2019-11-25-1574707306503

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 25, 2019
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Nov 25, 2019

Deploy preview for docusaurus-2 ready!

Built with commit e7db3f5

https://deploy-preview-2055--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Nov 25, 2019

Deploy preview for docusaurus-preview ready!

Built with commit e7db3f5

https://deploy-preview-2055--docusaurus-preview.netlify.com

@lex111
Copy link
Contributor Author

lex111 commented Nov 30, 2019

Hmm, for several days there has been no review from anyone, I think this is a good addition, don't you like it?

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

Oops.. I thought I've commented. So many PRs & issues this week that sometimes I miss one 😆

LGTM. but as always, gonna summon my UX & UI guru @wgao19 and @yangshun for approval

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

I think this is a useful addition, but I would classify it under "nice-to-have". Do you think it's possible to extract out all the new additions into a hook called useHideableNavbar?

Approved. Feel free to self-merge.

@endiliey
Copy link
Contributor

endiliey commented Dec 1, 2019

Docs needed

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

Friendly ping for docs & other refactor as suggested. Gonna skip it for this release

@lex111
Copy link
Contributor Author

lex111 commented Dec 9, 2019

Sorry for the delay, all proposals implemented (hook, docs).

@@ -90,6 +90,23 @@ module.exports = {

Outbound links automatically get `target="_blank" rel="noopener noreferrer"`.

### Auto-hide sticky navbar

You can enable this cool UI feature that automatically hides the navbar when a user starts scrolling down the page, and show it again when the user scrolls up.
Copy link
Contributor

Choose a reason for hiding this comment

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

it feels a bit weird that here we first address reader by "you" then later on say "a user" or "the user". I think it is ok to keep using you.

just personal opinion: i prefer it to be less objective. "cool" is also a very vague adjective and i found it a bit weird to read this from our docs

* LICENSE file in the root directory of this source tree.
*/

import React, {useState, useCallback, useEffect} from 'react'; // eslint-disable-line no-unused-vars
Copy link
Contributor

Choose a reason for hiding this comment

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

why need eslint disable ? is it React :O

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

LGTM

@yangshun yangshun merged commit 5bfa5d6 into facebook:master Dec 12, 2019
@endiliey endiliey added the pr: new feature This PR adds a new API or behavior. label Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants