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

Move sidebar logic to theme #3133

Closed
wants to merge 7 commits into from
Closed

Move sidebar logic to theme #3133

wants to merge 7 commits into from

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Dec 27, 2021

#3143 was originally part of this, but moved to make reviewing each individual feature easier.

This PR moves all our sidebar logic to our new reusable Gatsby theme.

Since the sidebars and logic have to be imported in both Gatsby Webpack and plain node environments while still being reusable, the sidebar leans on Gatsby Themes but in such a way that it can work in regular node. To help make this possible, sidebar normalization and helpers have been split and the helpers must be called on an imported instance of the sidebar- this helps particularly with plain Node.

@shcheklein shcheklein temporarily deployed to dvc-org-sidebar-to-plug-prlusp December 27, 2021 22:22 Inactive
@rogermparent rogermparent marked this pull request as draft December 27, 2021 23:26
@rogermparent rogermparent self-assigned this Dec 27, 2021
@rogermparent rogermparent temporarily deployed to dvc-org-sidebar-to-plug-prlusp December 28, 2021 00:43 Inactive
@rogermparent rogermparent temporarily deployed to dvc-org-sidebar-to-plug-prlusp December 29, 2021 01:40 Inactive
@rogermparent rogermparent temporarily deployed to dvc-org-sidebar-to-plug-prlusp December 29, 2021 01:44 Inactive
@rogermparent rogermparent temporarily deployed to dvc-org-sidebar-to-plug-prlusp December 29, 2021 22:36 Inactive
gatsby-config.js Outdated Show resolved Hide resolved
plugins/gatsby-theme-iterative-docs/gatsby-config.js Outdated Show resolved Hide resolved
Comment on lines -125 to +88
function normalizeSidebar({
function normalizeSidebar(
data,
parentPath,
parentResultRef,
startingPrevRef
}) {
{ parentPath = '', parentResultRef, startingPrevRef } = {}
) {
Copy link
Contributor Author

@rogermparent rogermparent Dec 29, 2021

Choose a reason for hiding this comment

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

Small tweak to the signature to make this function more natural to use IMO, the rest of the args are only used in recursive calls so the function's practically unary.

@@ -0,0 +1 @@
module.exports = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just here to be shadowed

Comment on lines 35 to +44
interface ISidebarMenuItemProps {
children?: Array<{ label: string; path: string; source: boolean | string }>
label: string
path: string
source: boolean | string
item: INormalizedSidebarItem
onClick: (isLeafItemClicked: boolean) => void
activePaths?: Array<string>
type?: string
style?: string
icon?: string
}

const SidebarMenuItem: React.FC<ISidebarMenuItemProps> = ({
children,
label,
path,
item: { children, label, path, style, icon, type },
activePaths,
onClick,
style,
icon,
type
onClick
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Slight signature tweak which makes more sense to me now that we have a type def for sidebar items.

Comment on lines +1 to +4
/* eslint-disable @typescript-eslint/no-var-requires */
const normalizeSidebar = require('../../plugins/gatsby-theme-iterative-docs/normalize-sidebar')
const sidebar = require('../../content/docs/sidebar.json')
module.exports = normalizeSidebar(sidebar)
Copy link
Contributor Author

@rogermparent rogermparent Dec 29, 2021

Choose a reason for hiding this comment

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

This shadowed sidebar file must be present at this specific path and default export a normalized sidebar. It also must be ES5 format to be used in gatsby-node contexts like remark plugins and server redirects- while non-webpack contexts don't recognize shadows, they can still require this file directly.

@rogermparent rogermparent temporarily deployed to dvc-org-sidebar-to-plug-prlusp December 29, 2021 23:04 Inactive
@rogermparent rogermparent temporarily deployed to dvc-org-sidebar-to-plug-prlusp December 29, 2021 23:06 Inactive
@rogermparent rogermparent temporarily deployed to dvc-org-sidebar-to-plug-prlusp December 29, 2021 23:12 Inactive
@rogermparent rogermparent marked this pull request as ready for review December 29, 2021 23:20
Copy link
Contributor

@yathomasi yathomasi left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Looks good!

@shcheklein
Copy link
Member

@rogermparent is it still a draft, should we merge this?

@rogermparent
Copy link
Contributor Author

Should we merge this?

Sorry about the lack of elaboration, #3170 takes the place of this and #3143 because it was easier to restart from master due to how these earlier PRs changed imports in many components.

@rogermparent rogermparent deleted the sidebar-to-plugin branch January 25, 2022 23:22
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.

4 participants