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(theme-classic): auto-collapse sibling categories in doc sidebar #3811

Merged
merged 15 commits into from
Jan 20, 2022

Conversation

softwarecurator
Copy link
Contributor

@softwarecurator softwarecurator commented Nov 24, 2020

Motivation

As a user having multiple opened categories in the sidebar, it got confusing and cluttered having that everything open on the left side of the screen.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Using the enabled themeConfig.autoCollapseSidebar option, you can make the sidebar only have one parent category open at a time, helping users not get cluttered and only focus on the content they selected. This is enabled by default, If you want them to be always opened, set themeConfig.autoCollapseSidebar to false:

module.exports = {
  // ...
  themeConfig: {
    autoCollapseSidebar: false,
    // ...
  },
};

Preview
sidebar

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.)

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 24, 2020
@netlify
Copy link

netlify bot commented Nov 24, 2020

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 976dd19

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61d9550b6b03490008a4c7f0

😎 Browse the preview: https://deploy-preview-3811--docusaurus-2.netlify.app

@softwarecurator softwarecurator marked this pull request as ready for review November 25, 2020 01:08
@slorber
Copy link
Collaborator

slorber commented Nov 25, 2020

Thanks, that looks like a useful new feature to add!

I'd rather not enable this by default without polling the community about that first.

Also, not totally fan of the mutations being done in the implementation. The collapsed state is currently in the category. If you want a category to collapse, it would be better to make sure that setCollapsed(true) is called.

I see 2 possible solutions:

Also the close animation seems to not work as reliably as before

@rylandg
Copy link

rylandg commented Dec 2, 2020

@slorber I'd love this feature, what is the current blocker?

@slorber
Copy link
Collaborator

slorber commented Dec 3, 2020

@rylandg the problem is written in the comment just before: #3811 (comment)

If you are able to get it working with:

  • a less hacky implementation (ie not mutating objects)
  • the animations working as before
  • opt-in, configurable, default to former behavior (no breaking change)

If you are unable to get it working, I'll figure it out myself and merge it, but I'm still busy working on i18n and have no time to work on it in the short term.

@github-actions
Copy link

github-actions bot commented Dec 18, 2020

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 84
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-3811--docusaurus-2.netlify.app/

@github-actions
Copy link

github-actions bot commented Dec 18, 2020

Size Change: +13 B (0%)

Total Size: 155 kB

ℹ️ View Unchanged
Filename Size Change
website/build/blog/2017/12/14/introducing-docusaurus/index.html 20.7 kB -1 B (0%)
website/build/docs/introduction/index.html 180 B 0 B
website/build/index.html 5.82 kB +1 B (0%)
website/build/main.********.js 111 kB +13 B (0%)
website/build/styles.********.css 17.6 kB 0 B

compressed-size-action

website/docs/sidebar.md Outdated Show resolved Hide resolved
@netlify

This comment has been minimized.

@softwarecurator
Copy link
Contributor Author

@rylandg the problem is written in the comment just before: #3811 (comment)

If you are able to get it working with:

  • a less hacky implementation (ie not mutating objects)
  • the animations working as before
  • opt-in, configurable, default to former behavior (no breaking change)

If you are unable to get it working, I'll figure it out myself and merge it, but I'm still busy working on i18n and have no time to work on it in the short term.

hey @slorber ! I have implemented 2/3 of the changes you required before it gets push but wanted to get you opinion on making the animation to correctly work, I brought all the state up so now the all the collapsing happens at the same time which I think changes how the animation worked since it waited 100ms between each state change. What would be the best way for the animation to work and to get this merged in?

Thank you!

@softwarecurator
Copy link
Contributor Author

just checking the status of this pull request @slorber thank you!

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Hi @josephriosIO , thanks for your work on this

There are a few things that do not look good to me:

  • A map with labels as index is not a reliable solution to store the categories states (it does not look simple as we don't have any unique category id, and probably need some kind of recursive structure)
  • The current UX looks weird to me (I guess it's quite subjective, maybe ask for feedbacks of your colleagues at Temporal about what they think about keeping the active category unfolded?)

For the animation, I didn't implement it and not 100% sure why the timeout was needed in the first place. I guess it could be restored by using a useEffect() somewhere, or maybe const isCollapsedDebounced = useDebounce(isCollapsed,100) ?

Also, it'd be nice to update your PR and solve the conflicts with master

Let me know if you need any help (I can commit to your PR, that may be faster for me than writing down my ideas as comments)

@Josh-Cena Josh-Cena changed the title Auto collapse sidebar v2 feat(theme-classic): auto-collapse sibling categories in doc sidebar Oct 28, 2021
@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Oct 28, 2021
@Josh-Cena Josh-Cena requested a review from slorber January 8, 2022 08:53
@Josh-Cena
Copy link
Collaborator

@slorber I think this is good enough? Our sidebar is too complicated, just used even more hooks inside ㄟ(▔,▔)ㄏ Context providers aren't the cleanest solution IMO, since we don't pass this to deep children... But so far it seems it works

@@ -111,6 +114,28 @@ function DocSidebarItemCategory({
});

useAutoExpandActiveCategory({isActive, collapsed, setCollapsed});
const {expandedItem, setExpandedItem} = useDocSidebarItemsExpandedState();
function updateCollapsed(toCollapsed: boolean = !collapsed) {
setExpandedItem(toCollapsed ? null : index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only case we have to think twice about UX is when there are multiple sidebar categories expanded by default. In this implementation, the expandedItem context is initialized to null, and when the user collapses one of those default-expanded categories, the others remain expanded until any category is toggled to be expanded.

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’m excited this feature is finally getting some steam

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes! This is the UX I've been looking forward to, so I did invest some time in this :D

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 haven’t really looked at the code, just a quick glance but I would set the default setting to false, I know not everyone will want this feature on by default, and that was an issue when I first tried to get this in over a year ago 😂

Copy link
Collaborator

@Josh-Cena Josh-Cena Jan 8, 2022

Choose a reason for hiding this comment

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

Yes, it's indeed defaulted to false :) Just that it would be cool to be turned on for our website.

@Josh-Cena Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Jan 8, 2022
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Alright, that's not the ideal implementation, but that seems good enough for now, so let's move on

The only case we have to think twice about UX is when there are multiple sidebar categories expanded by default. In this implementation, the expandedItem context is initialized to null, and when the user collapses one of those default-expanded categories, the others remain expanded until any category is toggled to be expanded.

That's true there are a few unhandled edge cases, but most users will likely not notice them.

We'll refactor this feature and make it more robust later.


Let's enable it on our own website for now to dogfood

But I'm very likely to turn this off later because for me it is a real pain to click on a category see it shift far away from your mouse cursor.

@slorber slorber merged commit 8ce3cee into facebook:main Jan 20, 2022
@slorber slorber removed the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Jan 20, 2022
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

8 participants