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

Refactoring getNavigationTree to include folders #105

Merged
merged 31 commits into from
Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
34eea76
draft: new folder struct
prestonbourne Jun 14, 2023
357b0ab
Add new Component to better view form capabilities
prestonbourne Jun 14, 2023
822b002
new nav data structure
prestonbourne Jun 14, 2023
50b9643
refactor ui to represent new nav data structure
prestonbourne Jun 14, 2023
2008875
enforce type safety
prestonbourne Jun 14, 2023
2b3123a
remove log
prestonbourne Jun 14, 2023
cad984c
prevent build failure
prestonbourne Jun 14, 2023
c366b49
add styles
prestonbourne Jun 15, 2023
f289666
formatting
prestonbourne Jun 15, 2023
1cb9e86
formatting
prestonbourne Jun 15, 2023
678197a
styling
prestonbourne Jun 15, 2023
b529bf8
clean up, remove unused
prestonbourne Jun 15, 2023
23f82c8
sorting function
prestonbourne Jun 15, 2023
448f6ee
styling
prestonbourne Jun 15, 2023
af5e17a
reshape tree structure
prestonbourne Jun 15, 2023
4761784
make `readonly`
prestonbourne Jun 15, 2023
f216ead
remove `readonly`
prestonbourne Jun 15, 2023
5d0001c
remove comment
prestonbourne Jun 15, 2023
ddbe5bb
formatting
prestonbourne Jun 15, 2023
37f21b2
formatting
prestonbourne Jun 15, 2023
5a3a1f3
remove `Omit`
prestonbourne Jun 15, 2023
1756b6d
add `__`
prestonbourne Jun 15, 2023
d70bf28
add `__`
prestonbourne Jun 15, 2023
3163c55
logic rewrite
prestonbourne Jun 15, 2023
c0217d8
rename to node
prestonbourne Jun 15, 2023
d52d6f2
use ternary over if
prestonbourne Jun 15, 2023
7cb1683
type fix
prestonbourne Jun 16, 2023
59fce61
return correct type
prestonbourne Jun 16, 2023
0a0ef7a
change variable name and remove `Object.values`
prestonbourne Jun 16, 2023
7176b92
destructure
prestonbourne Jun 16, 2023
cbbca16
--allow-empty-tag
prestonbourne Jun 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions examples/basic/components/checkbox/docs.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
title: 'Checkbox'
description: 'A checkbox'
path: 'Components/Forms/Checkbox'
---

import { Checkbox } from '.'

## Simple

<Checkbox />
3 changes: 3 additions & 0 deletions examples/basic/components/checkbox/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function Checkbox(props) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new components under the [Form] folder to better demonstrate sorting and collapsing.

return <input type="checkbox" {...props} />
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import { NavigationNode, ComponentNode } from 'swingset/types'
import { LinkItem } from './link-item'
import { cx } from 'class-variance-authority'

function Category({
title,
items,
}: {
title: string
items: NavigationNode[]
}) {
return (
<li className="ss-list-none">
<section className="ss-mb-4">
<CategoryHeading>{title.toUpperCase()}</CategoryHeading>
<ComponentList items={items} />
</section>
</li>
)
}
//Swap this out for already existing heading && Enquire about semantics, https://helios.hashicorp.design/components/application-state uses <div>
function CategoryHeading({ children }: { children: string }) {
return (
<div className="ss-uppercase ss-text-xs ss-font-semibold ss-leading-6 ss-text-foreground-faint ss-border-b ss-border-faint ss-pb-2">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently using a div for a heading because it's the pattern Helios uses.

{children.toUpperCase()}
</div>
)
}

function ComponentList({
prestonbourne marked this conversation as resolved.
Show resolved Hide resolved
isNested,
items,
}: {
isNested?: true
items: NavigationNode[]
}) {
return (
<ul
className={cx(
'ss-mt-2 ss-space-y-1',
isNested && 'ss-ml-2 ss-border-l ss-border-faint ss-mt-0'
)}
>
{items.map((item) => {
const isFolder = item.__type === 'folder'
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a good candidate for using a switch statement on __type. It's stylistic really, but I think it would make the logic of this block easier to understand.


if (isFolder) {
return (
<Folder key={item.title} title={item.title} items={item.children} />
)
}

const hasChildren = (item.children?.length as number) > 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to determine whether children can be undefined or an empty []. Currently the type is NavigationNode[] | undefined hence the typecast, but all the values resolve to []


if (hasChildren) {
return (
<li key={item.title}>
<LinkItem to={item.slug} title={item.title} />
<ComponentList
isNested
items={item.children as unknown[] as ComponentNode[]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This as unknown[] as ComponentNode[] typecast is a result of typescript expecting ComponentEntities instead of ComponentNodes. Which is unwanted behavior (see changes to types.ts)

Should be a fairly quick fix, see #107

Copy link
Contributor

Choose a reason for hiding this comment

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

Want to add a // TODO here linking to the issue so we know to clean this up as part of addressing that issue?

/>
</li>
)
}

return (
<li key={item.title}>
<LinkItem to={item.slug} title={item.title} />
</li>
)
})}
</ul>
)
}

function Folder({
title,
items,
}: {
title: ComponentNode['title']
items: ComponentNode[]
}) {
return (
<details>
<summary
className={cx(
'ss-text-foreground-primary hover:ss-text-foreground-action hover:ss-bg-surface-action',
'ss-rounded-md ss-p-2 ss-text-sm ss-leading-6 ss-cursor-pointer'
)}
>
{title}
</summary>
<ComponentList isNested items={items} />
</details>
)
}

export default Category
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { NavigationTree } from 'swingset/types'
import Category from './category'

type SideNavBarProps = {
categories: NavigationTree
}

function SideNavigation(props: SideNavBarProps) {
const { categories } = props
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, we can destructure this:

Suggested change
function SideNavigation(props: SideNavBarProps) {
const { categories } = props
function SideNavigation({ categories }: SideNavBarProps) {




const categoriesJSX = Object.values(categories).map(
(category) => (
<Category title={category.title} key={category.title} items={category.children} />
)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Object.values() will be unnecessary here if we fix the return value of getNavigationTree. In that case, we can use categories.map().


return <nav>{categoriesJSX}</nav>
}

export { SideNavigation }
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { Link } from "../link"
import { cx } from "class-variance-authority"

function LinkItem({ title, to }: Record<'title' | 'to', string>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of Record is clever here, but I think it'd be clearer if we used a more conventional type definition:

Suggested change
function LinkItem({ title, to }: Record<'title' | 'to', string>) {
type LinkItemProps = {
title: string
to: string
}
function LinkItem({ title, to }: LinkItemProps) {

return (
<Link
className={cx(
'ss-text-foreground-primary hover:ss-text-foreground-action hover:ss-bg-surface-action',
'ss-group ss-flex ss-gap-x-3 ss-rounded-md ss-p-2 ss-text-sm ss-leading-6 ss-no-underline'
)}
href={`swingset/${to}`}
>
{title}
</Link>
)
}

export {LinkItem}
53 changes: 4 additions & 49 deletions packages/swingset-theme-hashicorp/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,39 +4,10 @@ import '../style.css'
import React from 'react'
import Link from 'next/link'
import { meta, categories } from 'swingset/meta'
import { cx } from 'class-variance-authority'
import { SideNavigation } from './components/side-nav'

import Page from './page'

function NavList({ items, level }: any) {
const isNested = level > 0

return (
<ul
role="list"
className={cx(
'ss-mt-2 ss-space-y-1',
isNested && 'ss-ml-2 ss-border-l ss-border-faint ss-mt-0'
)}
>
{items.map(({ title, slug, children }: any) => (
<li key={slug}>
<Link
href={`/swingset/${slug}`}
className={cx(
'ss-text-foreground-primary hover:ss-text-foreground-action hover:ss-bg-surface-action',
'ss-group ss-flex ss-gap-x-3 ss-rounded-md ss-p-2 ss-text-sm ss-leading-6'
)}
>
{title}
</Link>
{children && <NavList level={level + 1} items={children} />}
</li>
))}
</ul>
)
}

export default function SwingsetLayout({
children,
}: {
Expand All @@ -45,7 +16,7 @@ export default function SwingsetLayout({
return (
<html lang="en" className="ss-h-full">
<body className="ss-h-full flex flex-col">
<div className="ss-hidden lg:ss-fixed lg:ss-inset-y-0 lg:ss-z-50 lg:ss-flex lg:ss-w-72 lg:ss-flex-col">
<aside className="ss-hidden lg:ss-fixed lg:ss-inset-y-0 lg:ss-z-50 lg:ss-flex lg:ss-w-72 lg:ss-flex-col">
<div className="ss-flex ss-grow ss-flex-col ss-gap-y-5 ss-overflow-y-auto ss-border-r ss-border-faint ss-bg-surface-faint ss-px-6 ss-py-10">
<div className="ss-flex ss-shrink-0 ss-items-center">
<Link
Expand All @@ -55,25 +26,9 @@ export default function SwingsetLayout({
Swingset
</Link>
</div>
<nav className="ss-flex ss-flex-1 ss-flex-col">
<ul
role="list"
className="ss-flex ss-flex-1 ss-flex-col ss-gap-y-7"
>
{Object.entries(categories).map(([title, items]) => (
<>
<li>
<h3 className="ss-uppercase ss-text-xs ss-font-semibold ss-leading-6 ss-text-foreground-faint ss-border-b ss-border-faint ss-pb-2">
{title}
</h3>
<NavList items={items} level={0} />
</li>
</>
))}
</ul>
</nav>
<SideNavigation categories={categories} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted all Nav logic into it's own folder in components/side-nav

</div>
</div>
</aside>
<main className="ss-py-10 lg:ss-pl-72 ss-flex ss-flex-col ss-flex-grow ss-h-full">
<div className="ss-px-4 sm:ss-px-6 lg:ss-px-8 ss-flex ss-flex-col ss-flex-grow">
{children}
Expand Down
6 changes: 6 additions & 0 deletions packages/swingset/src/default-theme/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ export default function SwingsetLayout({
<li>
<h3 className="ss-my-2 ss-text-gray-600">{title}</h3>
</li>
{/**
* TODO: Rewrite this for the new
* https://github.com/hashicorp/swingset/pull/105
* @type {NavigationTree}
* Leaving ts ignore to ensure build step succeeds
* @ts-ignore */}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue #106

{(items as string[]).map((slug: string) => (
<li>
<Link href={`/swingset/${slug}`}>{slug}</Link>
Expand Down
92 changes: 65 additions & 27 deletions packages/swingset/src/get-nav-tree.ts
Original file line number Diff line number Diff line change
@@ -1,49 +1,87 @@
import { ComponentEntity, DocsEntity } from './types.js'
import {
ComponentEntity,
DocsEntity,
ComponentNode,
CategoryNode,
} from './types.js'

export function getNavigationTree(entities: (ComponentEntity | DocsEntity)[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some unit tests for this function could be helpful! It'd make it easier for us to iterate on the logic going forward too. We can do this as a follow-up.

let result: Record<
string,
Pick<ComponentEntity, 'title' | 'slug' | 'componentPath' | 'children'>[]
> = {}

const componentEntities = entities.filter(
(entity) => entity.__type === 'component'
) as ComponentEntity[]

const componentEntitiesWithChildren = componentEntities.map((entity) => {
if (entity.isNested) return entity
const componentEntitiesWithChildren = componentEntities.map(
(componentEntity) => {
if (componentEntity.isNested) return componentEntity

entity.children = componentEntities.filter(
(childEntity) =>
childEntity.isNested &&
childEntity.componentPath === entity.componentPath
)
componentEntity.children = componentEntities.filter(
(childEntity) =>
childEntity.isNested &&
childEntity.componentPath === componentEntity.componentPath
)

return entity
})
return componentEntity
}
)

//TODO: Account for duplicate folder names [category]/[folder]
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting that this is applicable for folders, not necessarily categories

const categories = new Map<CategoryNode['title'], CategoryNode>()

// bucket components into categories, nested documents are categorized under their component's path
for (const entity of componentEntitiesWithChildren) {
if (entity.isNested) continue

//TODO: Handle Default Category
const category = entity.navigationData?.category!
const entityData: ComponentNode = {
__type: 'component',
title: entity.title,
slug: entity.slug,
componentPath: entity.componentPath,
children: entity.children,
}

const categoryTitle = entity.navigationData?.category || 'default'

let storedCategory =
categories.has(categoryTitle) && categories.get(categoryTitle)!

result[category] ||= []
//if category doesnt exist, create it
if (storedCategory === false) {
categories.set(categoryTitle, {
type: 'category',
title: categoryTitle,
children: [],
})
storedCategory = categories.get(categoryTitle)!
}

const folderTitle = entity.navigationData?.folder
const hasFolder = !!folderTitle
const folder = storedCategory.children.find(
(node) => node.title === folderTitle
)


result[category].push({
title: entity.title,
slug: entity.slug,
componentPath: entity.componentPath,
children: entity.children,
})
//if node belongs in a folder, and folder doesnt exist, create folder with node
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reduced nesting, should read like:

  1. If Category doesn't exist yet create it
  2. Check if current entity belongs in folder or not
  3. Add entity to folder if it already exists, if not create folder and add entity
  4. If entity doesn't have folder, add entity

if (hasFolder && !!folder === false) {
storedCategory.children.push({
__type: 'folder',
title: folderTitle,
parentCategory: categoryTitle,
children: [entityData],
})
continue
}

//if node belongs in a folder, and folder already exists, add node to folder
if (hasFolder && !!folder) {
folder.children ||= []
folder.children.push(entity)
continue
}

result[category].sort((a, b) => (a.slug > b.slug ? 1 : -1))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorting will be handled separately

//if node doesnt belong in folder, add node
storedCategory.children.push(entityData)
}

return result
const tree = Object.fromEntries(categories)
return tree
Copy link
Contributor

Choose a reason for hiding this comment

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

We want to return NavigationTree here, right? That's an array of CategoryNode, so we don't want an object here.

This might work (builds an array from the Map's values):

Array.from(categories.values())

}
5 changes: 1 addition & 4 deletions packages/swingset/src/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { compile, CompileOptions } from '@mdx-js/mdx'
import { VFile } from 'vfile'
import { matter } from 'vfile-matter'
import { type LoaderContext } from 'webpack'

import { resolveComponents } from './resolvers/component.js'
import { stringifyEntity } from './resolvers/stringify-entity.js'
import { getNavigationTree } from './get-nav-tree.js'
Expand Down Expand Up @@ -67,9 +66,7 @@ export async function loader(
const result = `
export const meta = {
${entities
.map(
(entity) => `'${entity.slug}': ${stringifyEntity(entity)}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prettier formatting fix

)
.map((entity) => `'${entity.slug}': ${stringifyEntity(entity)}`)
.join(',\n')}
};

Expand Down
4 changes: 2 additions & 2 deletions packages/swingset/src/meta.d.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
/**
* This file is a stub for a loader target and should never be imported directly
*/
import { EvaluatedEntity } from './types'
import { EvaluatedEntity, NavigationTree, ComponentEntity } from './types'

export const meta: Record<string, EvaluatedEntity>

export const categories: Record<string, string[]>
export const categories: NavigationTree

export function getEntity(slug: string): EvaluatedEntity | undefined

Expand Down
Loading