refactor(theme-{classic,common}): split navbar into smaller components + cleanup + swizzle config #6895

slorber · 2022-03-10T18:33:23Z

Breaking change

The theme Navbar components have been significantly updated so if you have swizzled them, you may need to re-eject them.

Motivation

Split Navbar into multiple smaller components, easier to swizzle
Move more technical code to theme-common

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

preview as before

netlify · 2022-03-10T18:41:29Z

✔️ [V2]

🔨 Explore the source changes: 44d447a

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

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

github-actions · 2022-03-10T18:41:51Z

⚡️ Lighthouse report for the changes in this PR:

Category	Score
🟠 Performance	57
🟢 Accessibility	100
🟢 Best practices	92
🟢 SEO	100
🟢 PWA	90

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

github-actions · 2022-03-10T18:43:44Z

Size Change: +2.58 kB (0%)

Total Size: 804 kB

Filename	Size	Change
`website/build/assets/css/styles.********.css`	104 kB	-35 B (0%)
`website/build/assets/js/main.********.js`	611 kB	+2.61 kB (0%)

ℹ️ View Unchanged

Filename	Size	Change
`website/.docusaurus/globalData.json`	49.9 kB	0 B
`website/build/index.html`	38.8 kB	+9 B (0%)

_{compressed-size-action}

packages/docusaurus-theme-common/src/utils/navbarUtils.ts

… state bugs

# Conflicts: # project-words.txt

slorber · 2022-03-12T15:52:06Z

ready for a review

we can keep components as unsafe for now, and eventually mark some as safe later after getting some user feedback?

Josh-Cena

The diff looks good to me! Wondering if this and its sisters should be marked as breaking changes since we changed the theme-common APIs?

Also, I think we can improve the swizzle CLI and ask the user to choose a subset of components (instead of always ejecting everything) in case of swizzling nested components?

Josh-Cena · 2022-03-13T00:27:55Z

packages/docusaurus-theme-classic/src/theme/Navbar/ColorModeToggle/index.tsx

+export default function NavbarColorModeToggle({
+  className,
+}: Props): JSX.Element | null {


I wonder if this layer of abstraction is useful? Supposing the entire purpose of ColorModeToggle is to toggle color modes, should we just move the hook calls to ColorModeToggle instead and remove this component?

There is value to have design system components (ColorModeToggle) to be very simple and have limited dependencies (ie not using any context). This makes it much easier to test then in isolation, add them to Storybook and other things that I plan to do.

This abstraction is likely to be temporary anyway if we provide a NavbarColorModeItem. For now I assume it's good enough, as this component is not marked as safe anyway

The general idea is that each call site should also be responsible to integrate the toggle on a case-by-case basis. For example, rendering null in case toggle is disabled might not be the best solution in all cases: it really depends on the parent component layout.

If we have later an option to have a sidebar color mode toggle, we'd create a similar SidebarColorModeToggle component, allowing the user to easily have distinct toggle designs for navbar/sidebar

I see, makes sense

Josh-Cena · 2022-03-13T00:29:38Z

packages/docusaurus-theme-classic/src/theme/Navbar/Content/index.tsx

+  return (
+    <NavbarContentLayout
+      left={
+        // TODO stop hardcoding items?


I think we should do that here, or at least in an immediate follow-up PR. I was asked about how we can change the color mode toggle position and the mobile toggle position. Also in https://docusaurus.canny.io/feature-requests/p/mobile-navbar-toggle-on-righttrailing-edge.

yes we should definitely give more flexibility here

It is a potential breaking change and probably requires some work so I'd rather have another follow-up PR to do so, eventually, find a way to be retro compatible (like normalizing the config and auto adding color mode toggle + search)?

About the mobile sidebar toggle, it probably doesn't make much sense to place it in other places than top right/left, but we can also give similar flexibility 🤷‍♂️

packages/docusaurus-theme-classic/src/theme/Navbar/MobileSidebar/SecondaryMenu/index.tsx

Josh-Cena · 2022-03-13T00:34:50Z

packages/docusaurus-theme-common/src/utils/navbarUtils.tsx

+  // TODO BAD, temporary type hack
+  T extends {position?: 'left' | 'right'},


Why is this bad? It looks pretty sufficient to me

If we didn't have the NavbarItem duplicate/incompatible types this wouldn't be necessary.

I'll keep the todo for now as IMHO we should only accept a NavbarItem type (unless we have another reason to use structural typing to split other kinds of items? In which case this util should be moved/renamed and made generic?)

packages/docusaurus-theme-common/src/utils/navbarUtils.tsx

slorber · 2022-03-16T15:55:44Z

The diff looks good to me! Wondering if this and its sisters should be marked as breaking changes since we changed the theme-common APIs?

I was also thinking about this.

In theory theme-common APIs are not documented and considered internal but at the same time it is a significant refactor that can break existing user code so not sure what to do 🤷‍♂️

Also, I think we can improve the swizzle CLI and ask the user to choose a subset of components (instead of always ejecting everything) in case of swizzling nested components?

yarn workspace website swizzle @docusaurus/theme-classic Navbar --eject --danger

Subfolders are not copied anymore by default, and I was thinking that an option --include-subfolders (+ prompt?) or something might be useful?

slorber · 2022-03-16T16:54:30Z

Note: I didn't include the NavbarItems in this refactor.

Should we nest everything related to the Navbar inside the navbar folder?

Should we create a "root" folder for all the major scopes of the theme?

Navbar
Footer
Docs
Blog
Pages
Components
DesignSystem
?

I think it can be useful but also quite disruptive for existing users, it may not have to happen immediately, we can keep this refactor for a later major version?

(I guess it's quite unlikely that we disagree on this FS structure anyway)

# Conflicts: # packages/docusaurus-theme-classic/src/theme/LayoutProviders/index.tsx # packages/docusaurus-theme-classic/src/theme/Navbar/index.tsx # packages/docusaurus-theme-common/src/utils/colorModeUtils.tsx

Josh-Cena · 2022-03-18T15:10:49Z

Should we create a "root" folder for all the major scopes of the theme?

I agree. "design system" would be for reusable components like paginators and breadcrumbs, right

slorber · 2022-03-18T15:17:01Z

"design system" would be for reusable components like paginators and breadcrumbs

yes, and we should try to encapsulate most of the Infima code there (apart things for spacing / layout etc)

slorber · 2022-03-18T15:17:45Z

going to merge this

it's not perfect and can't be marked as safe for swizzle, but still better than before

initial navbar refactor: move some hooks to theme-common

68246a4

slorber added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Mar 10, 2022

slorber requested review from lex111 and Josh-Cena as code owners March 10, 2022 18:33

facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 10, 2022

slorber marked this pull request as draft March 10, 2022 18:33

Josh-Cena reviewed Mar 11, 2022

View reviewed changes

packages/docusaurus-theme-common/src/utils/navbarUtils.ts Outdated Show resolved Hide resolved

slorber added 11 commits March 11, 2022 12:10

extract NavbarColorModeToggle

7bfefeb

extract NavbarMobileSidebar

b1b5d65

more navbar split/refactor

c8e928a

Merge branch 'main' into slorber/split-navbar

73aeb6b

stable navbar refactor

9deed0a

clear swizzle_theme_tests folder

e5008cf

minor refactor

c1a06a2

almost working refactor

f07baf1

refactor navbar secondary menu

87ceadf

merge navbar providers

65ef2d4

refactor useNavbarSecondaryMenu => hoist all state to avoid duplicate…

d5ddc81

… state bugs

slorber marked this pull request as ready for review March 11, 2022 18:58

slorber added 2 commits March 12, 2022 16:39

refactor useNavbarSecondaryMenu => hoist all state to avoid duplicate…

9470bd0

… state bugs

Merge branch 'main' into slorber/split-navbar

d1b44a4

# Conflicts: # project-words.txt

slorber requested a review from Josh-Cena March 12, 2022 15:51

Josh-Cena requested changes Mar 13, 2022

View reviewed changes

slorber added 2 commits March 16, 2022 16:29

Merge branch 'main' into slorber/split-navbar

ac0797a

move hook call to top-level

d90380e

slorber mentioned this pull request Mar 16, 2022

New Tailwind theme #2961

Open

slorber added 2 commits March 18, 2022 15:33

Merge branch 'main' into slorber/split-navbar

dfad62b

# Conflicts: # packages/docusaurus-theme-classic/src/theme/LayoutProviders/index.tsx # packages/docusaurus-theme-classic/src/theme/Navbar/index.tsx # packages/docusaurus-theme-common/src/utils/colorModeUtils.tsx

last updates

44d447a

slorber changed the title ~~refactor(theme-common): split navbar into smaller components + cleanup + swizzle config~~ refactor(theme-{classic,common}): split navbar into smaller components + cleanup + swizzle config Mar 18, 2022

slorber added the pr: breaking change Existing sites may not build successfully in the new version. Description contains more details. label Mar 18, 2022

slorber merged commit a1d333e into main Mar 18, 2022

slorber deleted the slorber/split-navbar branch March 18, 2022 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(theme-{classic,common}): split navbar into smaller components + cleanup + swizzle config #6895

refactor(theme-{classic,common}): split navbar into smaller components + cleanup + swizzle config #6895

slorber commented Mar 10, 2022 •

edited

netlify bot commented Mar 10, 2022 •

edited

github-actions bot commented Mar 10, 2022 •

edited

github-actions bot commented Mar 10, 2022 •

edited

slorber commented Mar 12, 2022

Josh-Cena left a comment •

edited

Josh-Cena Mar 13, 2022

slorber Mar 16, 2022

Josh-Cena Mar 17, 2022

Josh-Cena Mar 13, 2022

slorber Mar 16, 2022

Josh-Cena Mar 13, 2022

slorber Mar 16, 2022 •

edited

slorber commented Mar 16, 2022

slorber commented Mar 16, 2022 •

edited

Josh-Cena commented Mar 18, 2022

slorber commented Mar 18, 2022

slorber commented Mar 18, 2022

		// TODO BAD, temporary type hack
		T extends {position?: 'left' \| 'right'},

refactor(theme-{classic,common}): split navbar into smaller components + cleanup + swizzle config #6895

refactor(theme-{classic,common}): split navbar into smaller components + cleanup + swizzle config #6895

Conversation

slorber commented Mar 10, 2022 • edited

Breaking change

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

netlify bot commented Mar 10, 2022 • edited

github-actions bot commented Mar 10, 2022 • edited

github-actions bot commented Mar 10, 2022 • edited

slorber commented Mar 12, 2022

Josh-Cena left a comment • edited

Choose a reason for hiding this comment

Josh-Cena Mar 13, 2022

Choose a reason for hiding this comment

slorber Mar 16, 2022

Choose a reason for hiding this comment

Josh-Cena Mar 17, 2022

Choose a reason for hiding this comment

Josh-Cena Mar 13, 2022

Choose a reason for hiding this comment

slorber Mar 16, 2022

Choose a reason for hiding this comment

Josh-Cena Mar 13, 2022

Choose a reason for hiding this comment

slorber Mar 16, 2022 • edited

Choose a reason for hiding this comment

slorber commented Mar 16, 2022

slorber commented Mar 16, 2022 • edited

Josh-Cena commented Mar 18, 2022

slorber commented Mar 18, 2022

slorber commented Mar 18, 2022

slorber commented Mar 10, 2022 •

edited

netlify bot commented Mar 10, 2022 •

edited

github-actions bot commented Mar 10, 2022 •

edited

github-actions bot commented Mar 10, 2022 •

edited

Josh-Cena left a comment •

edited

slorber Mar 16, 2022 •

edited

slorber commented Mar 16, 2022 •

edited