-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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(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
Changes from all commits
68246a4
7bfefeb
b1b5d65
c8e928a
73aeb6b
9deed0a
e5008cf
c1a06a2
f07baf1
87ceadf
65ef2d4
d5ddc81
9470bd0
d1b44a4
ac0797a
d90380e
dfad62b
44d447a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import {useColorMode, useThemeConfig} from '@docusaurus/theme-common'; | ||
import ColorModeToggle from '@theme/ColorModeToggle'; | ||
import type {Props} from '@theme/Navbar/ColorModeToggle'; | ||
import React from 'react'; | ||
|
||
export default function NavbarColorModeToggle({ | ||
className, | ||
}: Props): JSX.Element | null { | ||
const disabled = useThemeConfig().colorMode.disableSwitch; | ||
const {colorMode, setColorMode} = useColorMode(); | ||
|
||
if (disabled) { | ||
return null; | ||
} | ||
|
||
return ( | ||
<ColorModeToggle | ||
className={className} | ||
value={colorMode} | ||
onChange={setColorMode} | ||
/> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import React, {type ReactNode} from 'react'; | ||
import type {Props as NavbarItemConfig} from '@theme/NavbarItem'; | ||
import NavbarItem from '@theme/NavbarItem'; | ||
import NavbarColorModeToggle from '@theme/Navbar/ColorModeToggle'; | ||
import SearchBar from '@theme/SearchBar'; | ||
import { | ||
splitNavbarItems, | ||
useNavbarMobileSidebar, | ||
useThemeConfig, | ||
} from '@docusaurus/theme-common'; | ||
import NavbarMobileSidebarToggle from '@theme/Navbar/MobileSidebar/Toggle'; | ||
import NavbarLogo from '@theme/Navbar/Logo'; | ||
import styles from './styles.module.css'; | ||
|
||
function useNavbarItems() { | ||
// TODO temporary casting until ThemeConfig type is improved | ||
return useThemeConfig().navbar.items as NavbarItemConfig[]; | ||
} | ||
|
||
function NavbarItems({items}: {items: NavbarItemConfig[]}): JSX.Element { | ||
return ( | ||
<> | ||
{items.map((item, i) => ( | ||
<NavbarItem {...item} key={i} /> | ||
))} | ||
</> | ||
); | ||
} | ||
|
||
function NavbarContentLayout({ | ||
left, | ||
right, | ||
}: { | ||
left: ReactNode; | ||
right: ReactNode; | ||
}) { | ||
return ( | ||
<div className="navbar__inner"> | ||
<div className="navbar__items">{left}</div> | ||
<div className="navbar__items navbar__items--right">{right}</div> | ||
</div> | ||
); | ||
} | ||
|
||
export default function NavbarContent(): JSX.Element { | ||
const mobileSidebar = useNavbarMobileSidebar(); | ||
|
||
const items = useNavbarItems(); | ||
const [leftItems, rightItems] = splitNavbarItems(items); | ||
|
||
const autoAddSearchBar = !items.some((item) => item.type === 'search'); | ||
|
||
return ( | ||
<NavbarContentLayout | ||
left={ | ||
// TODO stop hardcoding items? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 🤷♂️ |
||
<> | ||
{!mobileSidebar.disabled && <NavbarMobileSidebarToggle />} | ||
<NavbarLogo /> | ||
<NavbarItems items={leftItems} /> | ||
</> | ||
} | ||
right={ | ||
// TODO stop hardcoding items? | ||
// Ask the user to add the respective navbar items => more flexible | ||
<> | ||
<NavbarItems items={rightItems} /> | ||
<NavbarColorModeToggle className={styles.colorModeToggle} /> | ||
{autoAddSearchBar && <SearchBar />} | ||
</> | ||
} | ||
/> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
/* | ||
Hide color mode toggle in small viewports | ||
*/ | ||
@media (max-width: 996px) { | ||
.colorModeToggle { | ||
display: none; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import React, {type ComponentProps} from 'react'; | ||
import clsx from 'clsx'; | ||
import NavbarMobileSidebar from '@theme/Navbar/MobileSidebar'; | ||
import type {Props} from '@theme/Navbar/Layout'; | ||
import { | ||
useThemeConfig, | ||
useHideableNavbar, | ||
useNavbarMobileSidebar, | ||
} from '@docusaurus/theme-common'; | ||
|
||
import styles from './styles.module.css'; | ||
|
||
function NavbarBackdrop(props: ComponentProps<'div'>) { | ||
return ( | ||
<div | ||
role="presentation" | ||
{...props} | ||
className={clsx('navbar-sidebar__backdrop', props.className)} | ||
/> | ||
); | ||
} | ||
|
||
export default function NavbarLayout({children}: Props): JSX.Element { | ||
const { | ||
navbar: {hideOnScroll, style}, | ||
} = useThemeConfig(); | ||
const mobileSidebar = useNavbarMobileSidebar(); | ||
const {navbarRef, isNavbarVisible} = useHideableNavbar(hideOnScroll); | ||
return ( | ||
<nav | ||
ref={navbarRef} | ||
className={clsx( | ||
'navbar', | ||
'navbar--fixed-top', | ||
hideOnScroll && [ | ||
styles.navbarHideable, | ||
!isNavbarVisible && styles.navbarHidden, | ||
], | ||
{ | ||
'navbar--dark': style === 'dark', | ||
'navbar--primary': style === 'primary', | ||
'navbar-sidebar--show': mobileSidebar.shown, | ||
}, | ||
)}> | ||
{children} | ||
<NavbarBackdrop onClick={mobileSidebar.toggle} /> | ||
<NavbarMobileSidebar /> | ||
</nav> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import React from 'react'; | ||
import Logo from '@theme/Logo'; | ||
|
||
export default function NavbarLogo(): JSX.Element { | ||
return ( | ||
<Logo | ||
className="navbar__brand" | ||
imageClassName="navbar__logo" | ||
titleClassName="navbar__title" | ||
/> | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toColorModeToggle
instead and remove this component?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, makes sense