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

[FEAT] Refactors Page service to support nested routes #21

Merged
merged 1 commit into from
Sep 24, 2019

Conversation

pzuraq
Copy link

@pzuraq pzuraq commented Sep 24, 2019

This PR refactors the page service to allow it to support nested routes.
The guiding principle here is that our UX for traversing the guides is
a depth-first traversal of the docs tree, so rather than traversing the
tree directly each time, we do a single traversal once, creating both a
convenient data structure for looking up pages based on URL, and setting
up the next/prev links between each page.

Should likely add some tests around the nextPage and previousPage
and their sections as well, to ensure that next/prev page work with
arbitrary navigation trees.

@pzuraq
Copy link
Author

pzuraq commented Sep 24, 2019

Note that this is technically a breaking change, since there will never be a null or undefined next/previous page with this setup. Pages essentially act like a linked list, and they always have the information for the next/previous link in the chain with them. I would probably release this in a v2.0.0 because of this.

This PR refactors the page service to allow it to support nested routes.
The guiding principle here is that our UX for traversing the guides is
a depth-first traversal of the docs tree, so rather than traversing the
tree directly each time, we do a single traversal once, creating both a
convenient data structure for looking up pages based on URL, and setting
up the next/prev links between each page.

Should likely add some tests around the `nextPage` and `previousPage`
and their sections as well, to ensure that next/prev page work with
arbitrary navigation trees.
Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

Ahh this is amazing, we have needed this for a long time. Thank you!! 👍

@MelSumner MelSumner merged commit 0790e9b into empress:master Sep 24, 2019
@mansona
Copy link
Member

mansona commented Sep 25, 2019

So it looks like this has actually broken something 🙈 I notice on the deploy preview https://deploy-preview-21--guidemaker.netlify.com/release/ it shows the next page but when you click it then the previous page doesn't show up 🤔 it's something that we might need to back out and re-open the PR

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.

None yet

3 participants