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): add ability to hide doc sidebar #3615

Merged
merged 3 commits into from
Oct 22, 2020
Merged

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Oct 20, 2020

Motivation

Redesigned and improved (based on feedback) version of #2872.

The animation (collapse/expand) is implemented in two stages, first we change sidebar width (hiddenSidebarContainer state), and then we change the visibility of sidebar content (hiddenSidebar state). This way, we have smooth and neat animation effect.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview.

ezgif com-gif-maker (8)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Oct 20, 2020
@lex111 lex111 requested a review from slorber as a code owner October 20, 2020 22:30
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 20, 2020
@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-2 failed.

Built with commit 485419c

https://app.netlify.com/sites/docusaurus-2/deploys/5f8f64f225eeb40008acf9b7

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 20, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 02ef134

https://deploy-preview-3615--docusaurus-2.netlify.app

@lex111
Copy link
Contributor Author

lex111 commented Oct 20, 2020

Very strange errors when deploying this PR on Netlify.. @slorber could you please to check it out? This seems to be related to useContextualSearchFilters, but I don't understand how the changes in this PR affect search 🤔

@slorber
Copy link
Collaborator

slorber commented Oct 21, 2020

It looks like it's the v1-migrated deploy preview deployment that fail (maybe others are failing too as they are built later)

There is a command yarn netlify:deployPreview:v1-migrated to migrate v1 and try to build it.
Once migrated you can also try yarn workspace docusaurus-1-website-migrated start to start in dev mode.

Not sure what is happening exactly but it seems clearly related to the @theme-init alias. Removing it + the file loaders make it pass.

Not sure why the regular v2 site build does not fail, but I think we don't have a deterministic order for the created webpack aliases, and plugin/theme ordering may have an impact on the alias ordering and resolution.

I think for now using @theme-init alias may be risky, at least I don't know all the implications it could have. If we add this alias maybe we should ensure it ends up being the last in the alias list or something.

@slorber
Copy link
Collaborator

slorber commented Oct 21, 2020

btw the alias does not seem required to import the files without converting them to SVGs.

This seems to work fine with:

    background: url('~!file-loader!../icons/arrow.svg') no-repeat
      center center;

Did you have an idea in mind for this alias?

@lex111
Copy link
Contributor Author

lex111 commented Oct 21, 2020

I don't like relative imports, it looks awful for me, so I wanted to create the generic alias and use it. But strange that such the alias doesn't work for migrated-v1 site, although in case of v2 website build it works fine.

Okay, I'll probably have to use "ugly" relative imports. 😢

@slorber
Copy link
Collaborator

slorber commented Oct 21, 2020

understand :) also don't like relative paths much, but as they are checked by webpack at build time it's pretty safe that they won't break.


The feature seems to work great.

The transition is a bit weird to me, as the button moves from bottom to middle in a not very smooth way.

Wonder if it wouldn't be better to fully collapse the sidebar and have a little button appearing a bit later with an opacity effect? Maybe one of the suggestions of @yangshun on the original issue:

image

As it's subjective/design maybe we can ask for some community feedback on discord and see what users think.

Also wonder if the sidebar state should be persisted or not.

@lex111
Copy link
Contributor Author

lex111 commented Oct 21, 2020

The transition is a bit weird to me.

What exactly is wrong?

Wonder if it wouldn't be better to fully collapse the sidebar and have a little button appearing a bit later with an opacity effect?

Hmm, I think it is quite small button in that design, so I prefer clickable panel at full height because it is more visible and convenient to interact with. Feel free do survey, if you want.

Also wonder if the sidebar state should be persisted or not.

We definitely shouldn't store this state in local storage, in fact this functionality is relevant on some pages with lot of text (or on devices with medium screen size like tablets).

@slorber
Copy link
Collaborator

slorber commented Oct 22, 2020

as it's opt-in, let's merge and see if users like it ;)

@Simek
Copy link
Contributor

Simek commented Oct 23, 2020

@lex111 Can this be also implemented for the blog sidebar for the feature parity? Refs: #3628

@slorber
Copy link
Collaborator

slorber commented Oct 23, 2020

@Simek the blog has a significantly different layout compared to the docs. Not sure it's possible easily without rethinking the whole blog layout. Currently. it remains centered on purpose, no matter the presence of a sidebar or toc.

@lex111
Copy link
Contributor Author

lex111 commented Oct 23, 2020

I agree with @slorber, the blog sidebar cannot be made collapsible in its current design, it must at least be moved on the left side of page like by analogy doc sidebar. But I don't think we need to do this.

@lex111 lex111 deleted the lex111/hideable-sidebar-v2 branch October 23, 2020 18:43
@nategiraudeau
Copy link
Contributor

nategiraudeau commented Oct 30, 2020

I don't think that the current transition looks very good and I think it would look better if the expand button animated in when it's collapsed instead of just appearing.

I also don't like the e-resize cursor when it is collapsed, it makes me think that the sidebar is draggable; Since it's a button that expands the sidebar and not a draggable element, I think a pointer cursor would be better UX.

@Simek
Copy link
Contributor

Simek commented Nov 13, 2020

@lex111 The hideable sidebar is not working correctly on Windows currently (Chrome, Edge, FF, Opera). I can check this on macOS later, if you need that confirmation too.

I can hide the sidebar, but I'm not able to expand it back until I press on the small, visible part of sidebar entry. After the navigation is completed the expand button appears and I can expand the sidebar back.

Preview

sidebar

@slorber
Copy link
Collaborator

slorber commented Nov 13, 2020

not sure @lex111 but afaik onTransitionEnd might not be 100% reliable, I think this is an issue React had with CSSTransitionGroup

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.

None yet

6 participants