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

refactor(www): refactor and simplify the sidebar #23248

Merged
merged 24 commits into from
May 1, 2020
Merged

Conversation

tesseralis
Copy link
Contributor

@tesseralis tesseralis commented Apr 18, 2020

Description

Refactor and simplify the logic of the sidebar.

Changes:

  • Create SidebarContext to store common data so we don't have to keep passing down components
  • Accordion is now a function component whose uid is generated from item.title.
  • Fix the hacky useCallback in <Item> used to scroll to the right item.
  • Simplify a lot of duplicated logic for calculating active items/parents.
  • get rid of unused isSingle prop

TODO:

  • test the build to make sure everything actually works
  • Perf test: Make sure things remain "snappy". Adding context + memoization can cause things to become slow in unexpected ways.

Motivation

  • Clean up one of the most complex parts of the codebase.
  • Prepare for an optimized sidebar for translation.

@tesseralis tesseralis requested a review from a team as a code owner April 18, 2020 00:24
@tesseralis tesseralis requested a review from fk April 18, 2020 00:24
@tesseralis tesseralis changed the title refactor(www): refactor and simplify the sidebar refactor(www): WIP refactor and simplify the sidebar Apr 18, 2020
@tesseralis tesseralis requested a review from pieh April 18, 2020 02:19
@tesseralis tesseralis changed the title refactor(www): WIP refactor and simplify the sidebar refactor(www): refactor and simplify the sidebar Apr 18, 2020
@tesseralis
Copy link
Contributor Author

@fk There is one small UI change and I'm not sure if the original is intended or an error.

If you're on a page inside a top-level section (for example https://www.gatsbyjs.org/docs/recipes/working-with-plugins/) and close out its parent section, the top level looks like this:

Gatsbyjs.org collapsed sidebar with top level active section 'Recipes' in black text

But if you do the same thing to a deeper page (e.g. https://www.gatsbyjs.org/docs/sourcing-from-contentful/), its parent is rendered in purple:

Gatsbyjs.org collapsed sidebar with section 'Sourcing from (Headless) CMS' rendered in purple

I changed it so that the top level collapsed sections are in purple as well:

Gatsbyjs.org collapsed sidebar with top level active section 'Recipes' in purple text

@fk
Copy link
Contributor

fk commented Apr 26, 2020

I changed it so that the top level collapsed sections are in purple as well:

👍 🙏 Another great catch, Nat! 🙏
LG, and makes sense TM! ;).
(Esp. since it seems to be the only case where we're inconsistent:
image)

Copy link

@AishaBlake AishaBlake left a comment

Choose a reason for hiding this comment

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

Looking good! These changes make sense to me. Way to leave things better than you found them! :D

@AishaBlake AishaBlake merged commit c84e6ac into master May 1, 2020
@delete-merged-branch delete-merged-branch bot deleted the sidebar-refactor branch May 1, 2020 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants