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

fix(v2): fix FOUC in doc sidebar and various improvements #2867

Merged
merged 3 commits into from
Jun 2, 2020

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jun 2, 2020

Motivation

CC @jsjoeio @lex111 @yangshun , this is a fix for #2682 (comment)

I investigated the reasons of the FOUC on the docs sidebar after adding the category.collapsed = false, but actually found multiple problems that lead me to this refactor (some of them existed before).

  • Reading window.location to know which sidebar items are "active": that works only client-side after hydration, and was a reason of the FOUC. Replaced by activePath that's actually provided on both client and server (via ReactRouter ). Note it could have been possible to use useLocation from React-router as well.

  • Bad normalization of the new category collapsed attribute. We want to ensure to always have a mandatory boolean for this setting in the metadata, otherwise we'll have to to const collapsed = maybeCollapsed ?? true everywhere.

  • Non-recursive active sidebar item, which makes the Guides not being highlighted here

image

  • Dangerous mutations during render. On the server, JS objects are not "reset" between page renders, so mutating the sidebar objects/props on one page actually affected the rendering of all other subsequent pages.
if (item.collapsed !== prevCollapsedProp) {
  setPreviousCollapsedProp(item.collapsed);
  setCollapsed(item.collapsed);
}
if (sidebarCollapsible) {
  sidebarData.forEach(sidebarItem =>
    mutateSidebarCollapsingState(sidebarItem, path)
  );
}
  • Categories and links melted. This leads to declaring a collapsing state in links while collapsing is only affecting categories. I've splitted this way:
function DocSidebarItem(props) {
  switch (props.item.type) {
    case "category":
      return <DocSidebarItemCategory {...props} />;
    case "link":
    default:
      return <DocSidebarItemLink {...props} />;
  }
}

After this refactor, I think the collapsed feature works as intended and the code is cleaner than before.

Please tell me if you see a breaking change or issue with existing behavior.

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

docusaurus-bot commented Jun 2, 2020

Deploy preview for docusaurus-2 ready!

Built with commit a3d7517

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

@yangshun yangshun changed the title Refactor DocsSidebar fix(v2): refactor DocsSidebar Jun 2, 2020
@slorber slorber changed the title fix(v2): refactor DocsSidebar fix(v2): refactor DocSidebar Jun 2, 2020
@@ -9,30 +9,25 @@ module.exports = {
docs: [
{
type: 'category',
collapsed: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed on purpose, to test that the normalization process actually fallbacks to true

);
}
return false;
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can detect active items with infinite category levels

sidebarData.forEach((sidebarItem) =>
mutateSidebarCollapsingState(sidebarItem, path),
);
}
Copy link
Collaborator Author

@slorber slorber Jun 2, 2020

Choose a reason for hiding this comment

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

very dangerous, particularly on the server, as mutating any object of sidebarData would affect other pages rendering

@lex111 lex111 changed the title fix(v2): refactor DocSidebar fix(v2): fix FOUC in doc sidebar and various improvements Jun 2, 2020
Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Unfortunately, the sidebarCollapsible option (when sidebarCollapsible = false) became broken after this refactoring.

Current Should be
image image

@slorber
Copy link
Collaborator Author

slorber commented Jun 2, 2020

thanks @lex111 that should be fixed now

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Awesome work! 👏

@slorber
Copy link
Collaborator Author

slorber commented Jun 2, 2020

thanks :)

@yangshun yangshun merged commit 6797af6 into facebook:master Jun 2, 2020
@jsjoeio
Copy link
Contributor

jsjoeio commented Jun 2, 2020

Wow! You're so quick. Fantastic work @slorber - thank you for cleaning up the small mess I made 😜 but seriously, you rock! 💯

@slorber
Copy link
Collaborator Author

slorber commented Jun 3, 2020

thanks 😃

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Jun 17, 2020
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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants