Implement MKTG-032 in DocsSidenav and DocsPage #154

zchsh · 2021-02-17T14:47:43Z

This PR refactors DocsSidenav to implement the related RFC, MKTG-032.

This change also bleeds into DocsPage, which has a significant refactor to align with the updated DocsSidenav API, and GlossaryPage, which is a less drastic but still breaking refactor to align with `DocsPage.

I've proven out these changes in hashicorp/packer#10656 and more recently hashicorp/packer#10716, so apart from unit tests, we have those PRs as examples of the refactored components working "in the wild". We could open further PRs on other docs sites to confirm the GlossaryPage compatibility, or rather the upgrade path.

Release Information

Please select one (required)

Major (This PR introduces a breaking (incompatible API) change)
Minor (This PR adds functionality but it backwards-compatible)
Patch (This PR adds a backwards-compatible bug fix)

Release Notes (required)

This release refactors `docs-sidenav`, `docs-page`, and `glossary-page` to implement [the MKTG-032 RFC](https://docs.google.com/document/d/1FPyWid2NmNI9E_6lH2UIMuEPqjqDCGe3F8wraPRQHQ8/edit#).

PR Checklist 🚀

Items in this checklist may not may not apply to your PR, but please consider each item carefully.

Add Asana and Preview links above.
Conduct thorough self-review.
Add or update tests as appropriate.
Write a useful description (above) to give reviewers appropriate context.
Add release notes to the appropriate section (above).
Conduct reasonable cross browser testing for both compatibility and responsive behavior (We have a Cross Browser Testing account for this, if you don't have access, just ask!).
Conduct reasonable accessibility review (use the WAS as a guide or an axe browser plugin until we establish more formal checks).
Identify (in the description above) and document (add Asana tasks on this board) any technical debt that you're aware of, but are not addressing as part of this PR.

vercel · 2021-02-17T14:47:45Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/hashicorp/react-components/GbPq6XqvzMzNP2dbwrEXYAkYhK8V
✅ Preview: https://react-components-git-zsdocs-sidenav-refactor-hashicorp.vercel.app

brkalow · 2021-02-22T15:38:08Z

packages/docs-sidenav/lib/fetch-github-file/github-query.js

@@ -0,0 +1,15 @@
+const fetch = require('isomorphic-unfetch')


This stuff (remote file fetching from GH) seems kind of specific to the packer registry implementation. 💭 I'm wondering if it makes sense to include the remote file fetching in this package. What do you think?

Great point - have moved this out into the Packer repo 👍

brkalow · 2021-02-23T15:28:24Z

packages/docs-sidenav/style.module.css

@@ -0,0 +1,237 @@
+@custom-media --mobile-viewports (max-width: 939px);


zchsh · 2021-03-06T04:54:00Z

@jescalan @brkalow welp, not exactly "this afternoon", apologies for the delay - I do feel like the extra time paid off in making things a bit more ship-shape.

Have also proven out the docs-page / docs-sidenav pre-releases pretty extensively in Packer work - hashicorp/packer#10716 has the most up-to-date approach.

Here are the current pre-releases:

@hashicorp/react-docs-page@10.9.4-alpha.53
@hashicorp/react-docs-sidenav@6.1.1-alpha.53
@hashicorp/react-glossary-page => 0.2.4-alpha.54

Let me know if there's any part of the PR I can add more detail on.

Side note: I want to try out the GlossaryPage pre-release - @brkalow are we using that in prod yet? Or is there a PR I could work off of to make sure it works?

zchsh · 2021-03-06T04:59:08Z

packages/docs-sidenav/index.js

+  const handleMenuTransitionEnd = useCallback(() => {
+    setIsMenuFullyHidden(!isMobileOpen)
+  }, [isMobileOpen, setIsMenuFullyHidden])
+  useEventListener('transitionend', handleMenuTransitionEnd, menuRef.current)


Had some fun trying to get the menu hide / show working as expected from both a visual and screen-reader standpoint. I think this approach works, and hopefully it doesn't feel too wild - I may have gone a bit overboard with the comments here.

zchsh · 2021-03-06T05:00:05Z

packages/docs-sidenav/index.js

+  // we want to close the mobile rather than keep it open
+  useEffect(() => {
+    setIsMobileOpen(false)
+  }, [pathname])


This fixes an issue where the mobile menu didn't close on client-side navigation - shoutout to @brkalow for suggesting this solution 🙇

zchsh · 2021-03-06T05:01:42Z

packages/docs-sidenav/index.js

+          content={filteredContent || []}
+          currentPath={currentPath}
+          Link={Link}
+        />


I've changed from a function here to a component, it just felt a little clearer when trying to map how the rendering plays out to how the nav-data is structured in terms of branch and leave nodes. The way things work is still really similar with the recursive rendering etc.

brkalow

Really appreciate the improvements and enhancements you made as part of this. The implementation looks solid. Great tests!

Way to leave it better than you found it ❤️

brkalow · 2021-03-06T19:07:38Z

packages/docs-page/index.js

-  order,
-  pagePath,
+  navData,
+  currentPath,


Love the clarity!

brkalow · 2021-03-06T19:07:48Z

packages/docs-page/index.js

-  mainBranch = 'main',
-  order,
-  pagePath,
+  navData,


brkalow · 2021-03-06T19:09:13Z

packages/docs-page/index.js

+  navData,
+  currentPath,
  pageTitle,
+  baseRoute,


I think baseRoute makes sense. I'll keep this in mind as I'm using the supbath terminology throughout the versioned docs work.

brkalow · 2021-03-06T19:09:54Z

packages/docs-page/index.test.js

+  }
+})
+
+describe('<DocsPage />', () => {


Amazing work!

brkalow · 2021-03-06T19:19:19Z

packages/docs-page/server.js

+  //  If it's not a landing page, then we search
+  // through our navData to find the node with a path
+  // that matches the pathArray we're looking for.
+  function flattenRoutes(nodes) {
+    return nodes.reduce((acc, n) => {
+      if (!n.routes) return acc.concat(n)
+      return acc.concat(flattenRoutes(n.routes))
+    }, [])
+  }
+  const allNodes = flattenRoutes(navData)


Potentially a future optimization, but we might consider moving this function to the top level and memoizing it so we only flatten the nav nodes once. I don't think we need to do it right away though!

brkalow · 2021-03-06T19:21:28Z

packages/docs-sidenav/bin/migrate-to-mktg-032.js

@@ -0,0 +1,206 @@
+#! /usr/bin/env node


This is great! We'll definitely need this for versioned docs transformation. I wonder where this should live with that in mind 🤔

See here for how I'm tentatively thinking of structuring transforms: https://github.com/hashicorp/mktg-versioned-docs/tree/main/src/transforms

brkalow · 2021-03-06T19:22:52Z

packages/glossary-page/index.js

      pageTitle="Glossary"
      product={{ name: product.name, slug: product.slug }}
-      subpath="docs"
+      baseRoute="docs"


Not used anywhere, yet. 😄

Co-authored-by: Bryce Kalow <bkalow@hashicorp.com>

jescalan

This is a crazy amount of work - very impressive. Thanks for taking this on!

jescalan · 2021-03-08T18:40:24Z

packages/docs-page/index.test.js

+    expect(document.title).toBe(`Test Page | Terraform by HashiCorp`)
+    // Confirm `baseRoute` and `navData` by checking for a rendered link
+    const activeLeaf = screen.getByText('AWS').parentNode
+    expect(activeLeaf.nodeName).toBe('A')


This is a dangerous reliance on html structure here - is there any way we can put a test id directly on the targeted element and select into that

@jescalan ack yeah good point... i was on the fence here since getByText felt right, but parentNode felt wrong.

I've updated to use screen.getByText('AWS').closest('a'), and then changed the assertion to ensure the href on the a tag is correct. Have made similar updates throughout docs-page and docs-sidenav tests 👍

jescalan · 2021-03-08T18:43:22Z

packages/docs-page/index.test.js

+    const contentParagraph = screen.getByText('This is a cool docs page!')
+    expect(contentParagraph.tagName).toBe('P')
+    // Confirm `product` is passed via class
+    const contentContainer = contentParagraph.parentNode.parentNode


Same here with the DOM traversal, I think this is something we want to try to avoid in tests if we can

Updated - now grabbing the .closest('article'). Test might break if content no longer renders an article, but at least less brittle than the current specificity 👍

jescalan · 2021-03-08T18:48:21Z

packages/docs-page/index.test.js

+    const jumpToSectionElem = screen.getByText('Jump to Section')
+    expect(jumpToSectionElem.tagName).toBe('SPAN')
+  })
+})


jescalan · 2021-03-08T18:57:20Z

packages/docs-page/server.js

+// docs catchall route parameters: route/[[...page]].jsx.
+// This default parameter ID captures that pattern.
+// It can be overridden via options.
+const DEFAULT_PARAM_ID = 'page'


jescalan · 2021-03-08T19:12:51Z

packages/docs-sidenav/bin/migrate-to-mktg-032.js

@@ -0,0 +1,206 @@
+#! /usr/bin/env node


I wonder that as well - maybe in the codemods repo?

jescalan · 2021-03-08T19:13:18Z

packages/docs-sidenav/docs.mdx

 ---

-This is a cool component for documentation
+DocsSidenav renders a tree of links, with the option to include nested sections. Horizontal dividers can also be included between items.


😂 you didn't like my original description?

zchsh changed the title ~~WIP - refactor DocsSidenav per RFC~~ [WIP] Refactor DocsSidenav per RFC Feb 17, 2021

vercel bot had a problem deploying to Preview February 17, 2021 16:48 Failure

vercel bot had a problem deploying to Preview February 18, 2021 17:35 Failure

brkalow reviewed Feb 22, 2021

View reviewed changes

vercel bot had a problem deploying to Preview February 23, 2021 13:44 Failure

vercel bot had a problem deploying to Preview February 23, 2021 13:48 Failure

brkalow reviewed Feb 23, 2021

View reviewed changes

vercel bot deployed to Preview February 25, 2021 14:50 View deployment

WIP initial refactor of DocsSidenav

813dfae

zchsh force-pushed the zs.docs-sidenav-refactor branch from 8b0d741 to 813dfae Compare February 25, 2021 15:09

vercel bot deployed to Preview February 25, 2021 15:09 View deployment

zchsh added 2 commits February 25, 2021 10:11

Reorder props for clearer diff

8bc3213

Add back useProductMeta

756e163

vercel bot deployed to Preview February 25, 2021 15:20 View deployment

Test out npx-based approach to migration script

455b8cc

vercel bot deployed to Preview February 25, 2021 18:20 View deployment

Remove devdeps, not useful

644398e

vercel bot deployed to Preview February 25, 2021 18:26 View deployment

Update comment

8620fb5

vercel bot deployed to Preview February 25, 2021 18:29 View deployment

Fix issue with direct links

87bc56c

vercel bot deployed to Preview February 25, 2021 20:48 View deployment

Refine DocsPage props API

a6ca716

vercel bot deployed to Preview February 25, 2021 21:32 View deployment

Update comment

d138f52

vercel bot deployed to Preview February 25, 2021 21:47 View deployment

Add todo

c53eb2c

vercel bot deployed to Preview February 25, 2021 22:12 View deployment

zchsh changed the title ~~[WIP] Refactor DocsSidenav per RFC~~ [WIP] Implement MKTG-032 in DocsSidenav and DocsPage Feb 25, 2021

zchsh requested a review from brkalow March 6, 2021 04:48

zchsh added 2 commits March 5, 2021 23:50

Bump docs-sidenav in docs-page

0cd39d4

Bump docs-page in glossary-page

96bd152

zchsh commented Mar 6, 2021

View reviewed changes

vercel bot deployed to Preview March 6, 2021 11:51 View deployment

brkalow approved these changes Mar 6, 2021

View reviewed changes

Fix glossary-page “Edit this page” link

661bbb3

Co-authored-by: Bryce Kalow <bkalow@hashicorp.com>

vercel bot deployed to Preview March 7, 2021 00:57 View deployment

jescalan approved these changes Mar 8, 2021

View reviewed changes

vercel bot deployed to Preview March 8, 2021 19:24 View deployment

vercel bot deployed to Preview March 8, 2021 19:38 View deployment

Fix issue with navData direct link edge case, add related test

8d8092c

zchsh force-pushed the zs.docs-sidenav-refactor branch from d219a3f to 8d8092c Compare March 8, 2021 19:39

zchsh added 2 commits March 8, 2021 14:40

Bump docs-sidenav in docs-page

36b794f

Bump docs-page in glossary-page

d860d4d

vercel bot deployed to Preview March 8, 2021 19:41 View deployment

Add LICENSE to glossary-page

0102034

vercel bot deployed to Preview March 8, 2021 19:43 View deployment

Rm migration scripts from docs-sidenav, moving to mktg-codemods repo

9ab4d62

vercel bot deployed to Preview March 8, 2021 20:47 View deployment

Update tests to avoid unstable parentNode selection

89f85ae

vercel bot deployed to Preview March 8, 2021 23:37 View deployment

zchsh merged commit ee24faf into main Mar 9, 2021

zchsh deleted the zs.docs-sidenav-refactor branch March 9, 2021 15:52

This was referenced Mar 9, 2021

chore: commit package-locks after fresh install #162

Merged

chore(docs-page): bump docs-sidenav to stable dep #163

Merged

chore(glossary-page): bump docs-page from pre-release to stable #164

Merged

chore(deps): update btn deps #168

Merged

zchsh mentioned this pull request Mar 18, 2021

website: Implement RFC MKTG-033 hashicorp/packer#10764

Merged

		@@ -0,0 +1,237 @@
		@custom-media --mobile-viewports (max-width: 939px);

Implement MKTG-032 in DocsSidenav and DocsPage #154

Implement MKTG-032 in DocsSidenav and DocsPage #154

Uh oh!

Conversation

zchsh commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Release Information

PR Checklist 🚀

Uh oh!

vercel bot commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zchsh commented Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brkalow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jescalan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zchsh commented Feb 17, 2021 •

edited

Loading

vercel bot commented Feb 17, 2021 •

edited

Loading

zchsh commented Mar 6, 2021 •

edited

Loading