Refactoring `getNavigationTree` to include folders #105

prestonbourne · 2023-06-14T22:08:17Z

Asana Task 🔗

What

Restructures exported member, categories from swingset/meta
Updates rendering logic in hashicorp-theme to match new NavigationTree data structure.

Why

Added functionality for Folder this way themes can easier integrate multifaceted components or sub components.
Allows for easier long term goal of sorting and searching functionality.

How

Rewrote getNavigationTree
Added new Node types within tree

packages/swingset-theme-hashicorp/src/components/side-nav/category.tsx

packages/swingset-theme-hashicorp/src/components/side-nav/index.tsx

BRKalow · 2023-06-15T14:03:51Z

packages/swingset/src/loader.ts

 import { resolveComponents } from './resolvers/component.js'
 import { stringifyEntity } from './resolvers/stringify-entity.js'
-import { getNavigationTree } from './get-nav-tree.js'
+import {  getNavigationTree } from './get-nav-tree.js'


Suggested change

import { getNavigationTree } from './get-nav-tree.js'

import { getNavigationTree } from './get-nav-tree.js'

BRKalow · 2023-06-15T14:06:36Z

packages/swingset/src/types.ts

+/* 
+Utility type to extract just necessary properties before going to theme, also renames '__type' key to type for external usage.
+Unwanted Behavior?? The children property comes through as a ComponentEntity, which exposes the entities to the theme/clients 
+*/
+export type ComponentNode = Omit<
+  Pick<
+    ComponentEntity,
+    '__type' | 'title' | 'slug' | 'componentPath' | 'children'
+  >,
+  '__type'
+> & { type: ComponentEntity['__type'] }


Can we remove the Omit here?

Suggested change

/*

Utility type to extract just necessary properties before going to theme, also renames '__type' key to type for external usage.

Unwanted Behavior?? The children property comes through as a ComponentEntity, which exposes the entities to the theme/clients

*/

export type ComponentNode = Omit<

Pick<

ComponentEntity,

'__type' | 'title' | 'slug' | 'componentPath' | 'children'

>,

'__type'

> & { type: ComponentEntity['__type'] }

/*

Utility type to extract just necessary properties before going to theme, also renames '__type' key to type for external usage.

Unwanted Behavior?? The children property comes through as a ComponentEntity, which exposes the entities to the theme/clients

*/

export type ComponentNode = Pick<

ComponentEntity,

"title" | "slug" | "componentPath" | "children"

> & { type: ComponentEntity["__type"] };

BRKalow · 2023-06-15T17:09:34Z

packages/swingset/src/get-nav-tree.ts

+
+
+
+    if (!!storedCategory) {


I would potentially split out the category creation (if not exists) from the folder handling. I think the flow would look like (in plain english):

check if category is stored if category is not stored, create the category object then, handle folder logic

This dedupes some of the folder logic between these two branches.

(optional) I also wonder if it'd be easier to build up the category map and folder map separately, and then after we're done iterating over all of the entities we can make sure each folder gets added to its proper category. that way we don't need to check each of a category's children for the correct folder when we're trying to add an element to it.

prestonbourne · 2023-06-15T16:16:15Z

packages/swingset/src/types.ts

+    '__type' | 'title' | 'slug' | 'componentPath' | 'children'
+  >,
+  '__type'
+> & { type: ComponentEntity['__type'] }


Removes __ from type so themes know to use this as a discriminator.

Extracting the children property without converting the type from ComponentEntity[] to ComponentNode[] is leading to issues with type inference. Currently using as unknown type cast. Will address in separate issue and PR. ComponentEntity should be ComponentNode in categories #107

prestonbourne · 2023-06-15T16:35:06Z

packages/swingset-theme-hashicorp/src/components/side-nav/category.tsx

+//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">


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

prestonbourne · 2023-06-15T16:43:55Z

packages/swingset-theme-hashicorp/src/components/side-nav/category.tsx

+              <LinkItem to={item.slug} title={item.title} />
+              <ComponentList
+                isNested
+                items={item.children as unknown[] as ComponentNode[]}


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

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

prestonbourne · 2023-06-15T16:44:57Z

examples/basic/components/checkbox/index.tsx

@@ -0,0 +1,3 @@
+export function Checkbox(props) {


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

prestonbourne · 2023-06-15T16:47:45Z

packages/swingset-theme-hashicorp/src/components/side-nav/category.tsx

+          )
+        }
+
+        const hasChildren = (item.children?.length as number) > 0


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 []

prestonbourne · 2023-06-15T16:50:38Z

packages/swingset/src/default-theme/index.tsx

+                       * https://github.com/hashicorp/swingset/pull/105
+                       * @type {NavigationTree}
+                       * Leaving ts ignore to ensure build step succeeds
+                       * @ts-ignore   */}


prestonbourne · 2023-06-15T19:05:30Z

packages/swingset/src/types.ts

+
+export type ComponentNode = Pick<
+  ComponentEntity,
+  '__type' | 'title' | 'slug' | 'componentPath' | 'children'


reverted back to __type

prestonbourne · 2023-06-15T19:08:35Z

packages/swingset/src/get-nav-tree.ts

-      componentPath: entity.componentPath,
-      children: entity.children,
-    })
+    //if node belongs in a folder, and folder doesnt exist, create folder with node


Reduced nesting, should read like:

If Category doesn't exist yet create it

Check if current entity belongs in folder or not

Add entity to folder if it already exists, if not create folder and add entity

If entity doesn't have folder, add entity

prestonbourne · 2023-06-15T19:09:43Z

packages/swingset/src/loader.ts

@@ -67,9 +66,7 @@ export async function loader(
    const result = `
      export const meta = {
        ${entities
-          .map(
-            (entity) => `'${entity.slug}': ${stringifyEntity(entity)}`


prettier formatting fix

prestonbourne · 2023-06-15T19:10:28Z

packages/swingset/src/get-nav-tree.ts


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


sorting will be handled separately

BRKalow

Looking good! I think the component breakdown you've come up with is sensible. There seems to be a mismatch between what we're returning from getNavigationTree and the type we're expecting (returning an object instead of CategoryNode[])

BRKalow · 2023-06-16T15:24:21Z

packages/swingset-theme-hashicorp/src/components/side-nav/category.tsx

+              <LinkItem to={item.slug} title={item.title} />
+              <ComponentList
+                isNested
+                items={item.children as unknown[] as ComponentNode[]}


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

BRKalow · 2023-06-16T15:26:08Z

packages/swingset/src/types.ts

+export type NavigationNode = ComponentNode | FolderNode
+
+export type CategoryNode = {
+  type: 'category'


Is __type the name we want to use? (I think yes based on your other comments)

Suggested change

type: 'category'

__type: 'category'

Originally the Omit<...> & {type: '....'} changed it but I saw in the first review you requested to remove the Omit.

I'm impartial on the naming but think type felt more consistent with the other properties.

The other two nodes use __type, so I'd say let's pick one or the other and be consistent between each interface 👍

Will stick with what's already there then, since __type is used across a few other files already

BRKalow · 2023-06-16T15:30:22Z

packages/swingset/src/get-nav-tree.ts

+  const tree = Object.fromEntries(categories)
+  return tree


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())

BRKalow · 2023-06-16T15:31:26Z

packages/swingset-theme-hashicorp/src/components/side-nav/index.tsx

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


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

BRKalow · 2023-06-16T15:31:51Z

packages/swingset-theme-hashicorp/src/components/side-nav/index.tsx

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


nit, we can destructure this:

Suggested change

function SideNavigation(props: SideNavBarProps) {

const { categories } = props

function SideNavigation({ categories }: SideNavBarProps) {

BRKalow · 2023-06-16T15:33:12Z

packages/swingset-theme-hashicorp/src/components/side-nav/link-item.tsx

+import { Link } from "../link"
+import { cx } from "class-variance-authority"
+
+function LinkItem({ title, to }: Record<'title' | 'to', string>) {


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) {

BRKalow · 2023-06-16T15:35:03Z

packages/swingset-theme-hashicorp/src/components/side-nav/category.tsx

+      )}
+    >
+      {items.map((item) => {
+        const isFolder = item.__type === 'folder'


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.

BRKalow · 2023-06-16T15:36:45Z

packages/swingset/src/get-nav-tree.ts

+    }
+  )
+
+  //TODO: Account for duplicate folder names [category]/[folder]


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

BRKalow · 2023-06-16T15:38:24Z

packages/swingset/src/get-nav-tree.ts

+  DocsEntity,
+  ComponentNode,
+  CategoryNode,
+} from './types.js'

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


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.

prestonbourne · 2023-06-16T16:26:21Z

packages/swingset/src/get-nav-tree.ts

+      /**TODO:
+       *  We should be pushing @type {ComponentNode} instead of @type {ComponentEntity}
+       *  https://github.com/hashicorp/swingset/issues/107
+       */


prestonbourne

Made the suggested adjustments. Definitely agree tests are the next step, also want to get Vercel previews working so you + over reviewers can reference it

prestonbourne added 4 commits June 13, 2023 22:54

draft: new folder struct

34eea76

Add new Component to better view form capabilities

357b0ab

new nav data structure

822b002

refactor ui to represent new nav data structure

50b9643

prestonbourne commented Jun 14, 2023

View reviewed changes

packages/swingset-theme-hashicorp/src/components/side-nav/category.tsx Show resolved Hide resolved

prestonbourne commented Jun 14, 2023

View reviewed changes

packages/swingset-theme-hashicorp/src/components/side-nav/index.tsx Outdated Show resolved Hide resolved

prestonbourne added 14 commits June 14, 2023 19:13

enforce type safety

2008875

remove log

2b3123a

prevent build failure

cad984c

add styles

c366b49

formatting

f289666

formatting

1cb9e86

styling

678197a

clean up, remove unused

b529bf8

sorting function

23f82c8

styling

448f6ee

reshape tree structure

af5e17a

make readonly

4761784

remove readonly

f216ead

remove comment

5d0001c

prestonbourne requested a review from BRKalow June 15, 2023 16:58

prestonbourne marked this pull request as ready for review June 15, 2023 16:58

BRKalow reviewed Jun 15, 2023

View reviewed changes

prestonbourne added 6 commits June 15, 2023 13:50

formatting

ddbe5bb

formatting

37f21b2

remove Omit

5a3a1f3

add __

1756b6d

add __

d70bf28

logic rewrite

3163c55

prestonbourne requested a review from BRKalow June 15, 2023 19:11

prestonbourne added 2 commits June 15, 2023 15:21

rename to node

c0217d8

use ternary over if

d52d6f2

prestonbourne commented Jun 16, 2023

View reviewed changes

BRKalow reviewed Jun 16, 2023

View reviewed changes

prestonbourne added 4 commits June 16, 2023 11:56

type fix

7cb1683

return correct type

59fce61

change variable name and remove Object.values

0a0ef7a

destructure

7176b92

prestonbourne commented Jun 16, 2023

View reviewed changes

prestonbourne requested a review from BRKalow June 16, 2023 16:35

--allow-empty-tag

cbbca16

prestonbourne merged commit 5898ca6 into main Jun 16, 2023

prestonbourne deleted the restructure-nav-data branch June 16, 2023 20:58

prestonbourne mentioned this pull request Jun 20, 2023

Sort NavigationTree alphabetically #112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring `getNavigationTree` to include folders #105

Refactoring `getNavigationTree` to include folders #105

prestonbourne commented Jun 14, 2023 •

edited

Loading

BRKalow Jun 15, 2023

BRKalow Jun 15, 2023

BRKalow Jun 15, 2023

prestonbourne Jun 15, 2023

prestonbourne Jun 15, 2023

prestonbourne Jun 15, 2023

BRKalow Jun 16, 2023

prestonbourne Jun 15, 2023

prestonbourne Jun 15, 2023

prestonbourne Jun 15, 2023

prestonbourne Jun 15, 2023

prestonbourne Jun 15, 2023

prestonbourne Jun 15, 2023

prestonbourne Jun 15, 2023

BRKalow left a comment •

edited

Loading

BRKalow Jun 16, 2023

BRKalow Jun 16, 2023

prestonbourne Jun 16, 2023

BRKalow Jun 16, 2023

prestonbourne Jun 16, 2023

BRKalow Jun 16, 2023

BRKalow Jun 16, 2023

BRKalow Jun 16, 2023

BRKalow Jun 16, 2023

BRKalow Jun 16, 2023

BRKalow Jun 16, 2023

BRKalow Jun 16, 2023

prestonbourne Jun 16, 2023

prestonbourne left a comment

	import { getNavigationTree } from './get-nav-tree.js'
	import { getNavigationTree } from './get-nav-tree.js'

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

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

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

Refactoring getNavigationTree to include folders #105

Refactoring getNavigationTree to include folders #105

Conversation

prestonbourne commented Jun 14, 2023 • edited Loading

What

Why

How

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BRKalow left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonbourne left a comment

Choose a reason for hiding this comment

Refactoring `getNavigationTree` to include folders #105

Refactoring `getNavigationTree` to include folders #105

prestonbourne commented Jun 14, 2023 •

edited

Loading

BRKalow left a comment •

edited

Loading