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

Sidebar Navigation for Reference Pages #50

Merged
merged 15 commits into from
May 18, 2020
Merged

Conversation

zstix
Copy link
Contributor

@zstix zstix commented May 12, 2020

Descriptions

Updates the reference page template to query GraphQL for markdown files. Once we have the data from Gatsby, we convert it to a format that our UI can use to build out a nested set of links.

The heavy lifting for this work is done in src/utils/nav-from-edges.js, so I added a bunch of documentation explaining what's going on. I also added a couple dummy markdown pages to help test that everything is working.

Note: in the future we should update the GraphQL query to only get files using the reference template (this gets all markdown files for now). Also, we are simply sorting alphabetically by displayName at the moment.

Preview Link(s)

Related Ticket(s)

Screenshot(s)

Screen Shot 2020-05-12 at 12 03 18 PM

@zstix zstix added this to the MMF 2 - Design & Navigation milestone May 12, 2020
Copy link
Contributor

@timglaser timglaser left a comment

Choose a reason for hiding this comment

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

This is a good case for a unit test on getNavFromEdges.

And I'm running into what might just call for a readme documentation in this project along the lines of "how to manage paths." I notice the presence of an /a/b path value keeps an /a/b/c path value from showing up as A => B => C in the nav even if an /a/b/index is present. I think writing out at least a draft documentation for this now will help solidify that this is feeling right.

@zstix
Copy link
Contributor Author

zstix commented May 13, 2020

Totally agree about the tests and about the documentation.

Also, it turns out that Gatsby doesn't treat paths that end with index as I would expect. Gatsby does not build a page like guides/index into developers.newrelic.com/guides. That might require a sizable refactor to this algorithm.

Probably something best done after we have some tests defining how we want it to function.

@zstix
Copy link
Contributor Author

zstix commented May 14, 2020

I've updated this PR to include a test to ensure that the utility function is running correctly. This also helped me refactor the link generation to resolve how we were creating "index" pages. Ready for a re-review!

@timglaser
Copy link
Contributor

Awesome work on this. Is the intention for this PR to support the following?

path 1: /a
path 2: /a/b1
path 2: /a/b2
path 3: /a/b1/c1
path 4: /a/b1/c2

I don't want to go overkill in testing/reviewing this if the existing test case in the test file is all we need at this moment for initial content creation.

@zstix
Copy link
Contributor Author

zstix commented May 14, 2020

@timglaser I believe that this test covers all of those scenarios. I'm happy to add more tests if we would like...was just trying to keep this moving.

Copy link
Contributor

@LizBaker LizBaker left a comment

Choose a reason for hiding this comment

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

🔗

@timglaser timglaser merged commit 2d55abc into master May 18, 2020
@timglaser timglaser deleted the zstickles/sidebar-nav branch May 18, 2020 23:11
@nr-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants