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): expanded sidebar categories by default #2682

Merged
merged 17 commits into from
May 28, 2020

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Apr 27, 2020

Motivation

This allows a user to toggle a category open by default.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Thanks to the tip from @lex111 I tested this locally on the Docusaurus website by modifying website/versioned_sidebars/version-2.0.0-alpha.40-sidebars.json and the changes worked as expected.

Screenrecording

2020-04-27 10 53 04

Updated: only highlight the category if the category belongs to the current page being viewed
2020-04-29 16 11 34

Related PRs

Remaining todos

  • update docs with example
  • link related PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 27, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Apr 27, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 4f2e6a4

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

Comment on lines 260 to 315
#### Toggling category open by default

For sites that have collapsible categories, you may want more fine grain control over certain categories. If you want specific categories to be alwasy expanded, you can set `collapsed` to `false`:

```js title="sidebars.js"
module.exports = {
docs: {
Guides: [
'creating-pages',
{
type: 'category',
label: 'Docs',
collapsed: false,
items: ['markdown-features', 'sidebar', 'versioning'],
},
],
},
};
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yangshun I updated the docs - open to suggestions! I wasn't sure if I needed to update any other areas of the docs. Let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks fine. What's the current default value for the collapsed field? The default should be true to preserve backward compatibility.

@jsjoeio jsjoeio marked this pull request as ready for review April 27, 2020 18:10
@jsjoeio jsjoeio requested review from lex111 and yangshun as code owners April 27, 2020 18:10
@lex111 lex111 changed the title feat: allow user to toggle sidebar open default feat(v2): allow user to toggle sidebar open default Apr 27, 2020
@lex111 lex111 added this to the v2.0.0-alpha.54 milestone Apr 27, 2020
@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Apr 27, 2020
Comment on lines 9 to 57
docs: [
{
type: 'category',
label: 'Docusaurus',
collapsed: false,
items: ['introduction', 'design-principles', 'contributing'],
},
{
type: 'category',
label: 'Getting Started',
collapsed: true,
items: ['installation', 'configuration'],
},
{
type: 'category',
label: 'Guides',
collapsed: true,
items: [
'creating-pages',
'styling-layout',
'static-assets',
{
Docs: ['docs', 'markdown-features', 'versioning'],
},
'blog',
'search',
'deployment',
'migrating-from-v1-to-v2',
],
},
{
type: 'category',
label: 'Advanced Guides',
collapsed: true,
items: ['using-plugins', 'using-themes', 'presets'],
},
{
type: 'category',
label: 'API Reference',
collapsed: true,
items: [
'cli',
'docusaurus-core',
'docusaurus.config.js',
'lifecycle-apis',
'theme-classic',
],
},
],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lex111 is this what you had in mind?

image

Copy link
Contributor

@lex111 lex111 Apr 28, 2020

Choose a reason for hiding this comment

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

@jsjoeio Yes, thanks, this is exactly what I had in mind, however, to achieve this it is necessary to change the sidebar structure (use an array instead of an object), but ok.

Hmm, should we highlight expanded by default category? In my understanding, we need to highlight the category in which we are currently (based on the current path).

For example, I am on the Guides -> Docs page, everything is fine, two related items (Guides and Docs-Introduction) are highlighted in green.
But at the same time, we also see that the top-level item with Docusaurus is also highlighted as activated. I don’t know for sure, but it seems that similar behavior could lead people astray so we don’t need to highlight the category of sidebar, which are expanded by default.

Any objections? cc @yangshun

image

Copy link
Contributor

Choose a reason for hiding this comment

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

We should only highlight the active one. Opening/close doesn't result in highlighting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see!

Okay, I assume that's part of this work then - if you want to request changes on this PR, then I get started and ping you when it's ready for re-review @lex111

@lex111 lex111 removed this from the v2.0.0-alpha.54 milestone Apr 28, 2020
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Apr 29, 2020

I made a flow chart to understand the logic behind the category highlighting.

docsidebar-category-highlight

I also found the place where I need to modify this logic:
https://github.com/facebook/docusaurus/blob/jsjoeio/toggle-sidebar-open-default/packages/docusaurus-theme-classic/src/theme/DocSidebar/index.js#L51

@@ -48,7 +68,8 @@ function DocSidebarItem({item, onItemClick, collapsible, ...props}) {
<a
className={classnames('menu__link', {
'menu__link--sublist': collapsible,
'menu__link--active': collapsible && !item.collapsed,
'menu__link--active':
collapsible && !item.collapsed && isCategoryOfActivePage(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I love the function call here, might be better to create a new variable and assign it to a function call and then use it here. Open to other ideas too

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be better to make the function a pure one and pass in the require parameters. That makes it easier to test and move around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! I'll refactor that

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Apr 30, 2020

Classic. I'll fix this
image

@jsjoeio jsjoeio requested a review from lex111 May 4, 2020 23:47
@lex111
Copy link
Contributor

lex111 commented May 10, 2020

Hmm, we have a little regression behavior compared to the current version. If you open other categories and try to go to any page in them, they will not automatically collapse as earlier (see screenshot)

Before:

screencast-v2 docusaurus io-2020 05 10-10_29_59

After (it is expected that only the Docusaurus category should always be expanded after go to another category):

screencast-deploy-preview-2682--docusaurus-2 netlify app-2020 05 10-10_30_47

I don’t know how critical this bug is or not, I want to hear the opinion from @yangshun. Maybe we should adopt a new behavior (when the categories do not collapse after navigating to another category).

@jsjoeio
Copy link
Contributor Author

jsjoeio commented May 13, 2020

Ah! I didn't consider that. I'll wait to hear from @yangshun before making any changes

@yangshun yangshun added this to the v2.0.0-alpha.55 milestone May 14, 2020
@lex111 lex111 removed this from the v2.0.0-alpha.55 milestone May 19, 2020
@jsjoeio
Copy link
Contributor Author

jsjoeio commented May 19, 2020

Bump @yangshun or @lex111 - I see one added it to a milestone and the other removed it.

Hoping to get an update. What's the status here? (Would love to finish any remaining work and get this merged)

(P.S. I do see that merge conflicts need to be resolved)

@yangshun
Copy link
Contributor

The reason we had the initial behavior is because if users opened a category that's nearer to the bottom of the sidebar, they would have to scroll down before they can access the new one. Collapsing all others categories helps a little bit with the issue of reducing the non-relevant vertical space needed. That said, this is not critical and I think there's no common industry practice. Spark AR docs has our current behavior but [Android docs].

Another issue that we might face here if we decide to stick with the current behavior is what we should do about the categories which are open by default but user decides to click on some other category. Do we close them or keep them open? If we keep them open that logic could be harder to maintain.

I'd prefer to stick with the current behavior if possible, assuming we have a feasible solution to the issue I raised above. WDYT?

@yangshun yangshun added this to the v2.0.0-alpha.56 milestone May 25, 2020
@jsjoeio
Copy link
Contributor Author

jsjoeio commented May 27, 2020

I'd prefer to stick with the current behavior if possible, assuming we have a feasible solution to the issue I raised above. WDYT?

I agree with you @yangshun . I prefer to keep the current behavior.

Let me investigate to see if I can fix this.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented May 27, 2020

image

Weird...I rebased and now I'm seeing this new error. Now I need an id for each item? I'll figure it out, posting here for my own notes

@lex111
Copy link
Contributor

lex111 commented May 27, 2020

@jsjoeio nope, we just renamed the document with id "docs" to "docs-introduction"

Docs: ['docs-introduction', 'markdown-features', 'versioning'],

https://github.com/facebook/docusaurus/pull/2682/files#diff-0983ed9e7518a0fbbe7806ed8166258cR31

@jsjoeio
Copy link
Contributor Author

jsjoeio commented May 27, 2020

Ah, got it. Thanks! That fixed it :)

I drew up a diagram to make sure I understood the expected behavior.
docusaurus-expanded

Now let's investigate to see how we might implement or fix the regression.

@jsjoeio
Copy link
Contributor Author

jsjoeio commented May 27, 2020

Hmmm.. okay after a few hours of investigating, I don't think it's possible (but happy to be proven otherwise) due to these two facts:

  1. we mutate sidebar state mutateSidebarCollapsingState (i.e. item.collapsed) directly
  2. we rely on the same value to determine whether or not a category should be expanded by default

Ideal World

  • Use item.collapsed for the actual state of the category being collapsed or not
  • Don't mutate directly
  • Use separate value like item.alwaysExpanded to separate new functionality from collapsed state

Regression -> Better UX?

I could be convinced otherwise, but I feel like the new behavior is fine, if not better for the user?

As a user, here's a peek into my thoughts:
"Docusaurus...cool cool. What's in 'Getting Started'?"
*expands category
"Not what I'm looking for. How about 'Guides'?"
*expands category
"Hmm...not that either. I'll go to 'Installation' and see what's there"

In this case, I am glad 'Getting Started' didn't automatically collapse because I was peeking around.

Summary

Leave it as is and "adopt a new behavior (categories do not collapse after navigating to another category)." WDYT? (no strong opinion here)

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.

Just tried it out, it works great! Thanks @jsjoeio! I removed the setting of collapsed on every sidebar item since it's redundant and I also wanted to test the default behavior.

Let's go with this new UX and label it as a breaking change. We'll revisit this UX if people complain :)

FWIW nextjs.org docs also doesn't collapse the other categories.

@yangshun yangshun added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label May 28, 2020
@yangshun yangshun changed the title feat(v2): allow user to toggle sidebar open default feat(v2): expanded sidebar categories by default May 28, 2020
@yangshun yangshun merged commit 07b9e9c into master May 28, 2020
@yangshun yangshun deleted the jsjoeio/toggle-sidebar-open-default branch May 28, 2020 05:17
@jsjoeio
Copy link
Contributor Author

jsjoeio commented May 28, 2020

I removed the setting of collapsed on every sidebar item since it's redundant and I also wanted to test the default behavior.

Good call! Glad to hear everything still works as expected!

Let's go with this new UX and label it as a breaking change. We'll revisit this UX if people complain :)

Good idea! And if they complain, send them my way, and I'm happy to work with you and the team to get something figured it that keeps everyone happy :)

Thank you @yangshun (and thanks a ton for the internal feedback! ❤️) and @lex111 for the help along the way! It felt really good working on this. Happy to see it merged! Cheers! 🎉

@lex111
Copy link
Contributor

lex111 commented Jun 1, 2020

@jsjoeio @yangshun Unfortunately, this feature brings with it an unpleasant effect. During server prerendering, all categories are expanded, this is noticeable when the page loads: the categories are displayed as expanded for a while, and when the page is loaded they are already collapsed (as it should be initially).

This is kind of FOUC, as we had with dark mode, but now a similar bug is observed with doc categories. See gif below:

screencast-v2 docusaurus io-2020 06 01-00_14_37

As it was before:

screencast-deploy-preview-2822--docusaurus-2 netlify app-2020 06 01-00_24_45

You can see for yourself if you open these pages and try reloading them.

Current behavior - https://v2.docusaurus.io/docs/next/migrating-from-v1-to-v2/
Previous behavior (before the PR was merged) - https://deploy-preview-2822--docusaurus-2.netlify.app/docs/next/migrating-from-v1-to-v2/

To fix this bug, we need to render the final result upon initial render, and not only after mounting. @jsjoeio could you look at that?

@lex111
Copy link
Contributor

lex111 commented Jun 1, 2020

@slorber do you want to help with this?

@slorber slorber self-assigned this Jun 1, 2020
@slorber
Copy link
Collaborator

slorber commented Jun 1, 2020

yes, I'll take a look at how to fix this

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Jun 1, 2020

You can see for yourself if you open these pages and try reloading them.

I can confirm that I am experiencing this FOUC as well :(

Thanks for taking this on @slorber. This comment from earlier in the discussion may help. Please @ if I can help

@slorber
Copy link
Collaborator

slorber commented Jun 2, 2020

Should be fixed by #2867

yangshun pushed a commit that referenced this pull request Jun 2, 2020
* bug(v2): fix active sidebar item detection logic (#2682 (comment))

* fix sidebar category collapsed normalization to make sure we always have a boolean after normalization

* fix sidebarCollapsible option
@slorber
Copy link
Collaborator

slorber commented Jun 18, 2020

Released in https://github.com/facebook/docusaurus/releases/tag/v2.0.0-alpha.58

@sedghi
Copy link

sedghi commented Jun 21, 2021

@slorber is it possible to completely disable and hide the collapse button on the bottom of the side bar? I didn't find the flag in the config

@slorber
Copy link
Collaborator

slorber commented Jun 22, 2021

yes @sedghi https://docusaurus.io/docs/sidebar#hideable-sidebar

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: breaking change Existing sites may not build successfully in the new version. Description contains more details. pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v2] Option to toggle docs category open by default
7 participants