-
Notifications
You must be signed in to change notification settings - Fork 11
Conversation
Exciting! Overall, I think the direction is great. @colleenmcginnis I have a few initial questions:
|
I can't think of any uses outside of
I'm open to opinions. Not all products have guides ordered in a chronological way, but maybe that doesn't really matter.
I thought about this too! I thought about maybe making it full-width, but in
Yes!! This was going to be my next step after getting your initial reaction. :) |
Great, that's my recommendation. In this way our design system is a little more opinionated to help keep our sites consistent. But I think we can still have all the tests and examples we need to make sure each subcomponent is covered.
This distinction between chronological guides and grouped guides makes sense. It sounds like NextPrevPage is ideal for just grouped guides, at least for now!
That makes sense! I'm in favor for keeping Feedback on these pages for now!
🤗 |
Ok @katydecorah this is ready for a closer look! (but not urgent) |
@katydecorah I'm not sure why the tests are still failing. They pass locally for me... |
@colleenmcginnis The tests seem to be pointing at this 404 path. Want to try removing this line?
If it continues to give grief, then it's likely an issue with the sitemap tests and I'll look into it and fix in a new PR. Also want to note, that the sitemap test will only run if the catalog site has been built and created the sitemap. Which can be tricky to test locally, since these test will only run if you had recently run |
Now it fails locally, but passes here!
I did do that recently! 💡 |
Ugh! I'll look into smoothing this out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking so good! I left several suggestions and questions. Let me know if you want to talk through anything.
return array.find((x) => x.title === 'Guides'); | ||
} | ||
|
||
export function getSubPages(navigation, pathname, frontMatter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have tests for this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also wondering if we should move this to the batfish navigation helper to take pressure off the frontend. For example, maybe we could add groups
to the navigation object:
{
"navTabs": {...},
"hierarchy": {...},
"groups": {
"/dr-ui/guides/grouped-guides/": [ /* array of objects of page data: title, url, description */]
}
}
Then we can use something like the findParentPath()
function then navigation.groups[parentPath]
to return the list of subpages for the group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely open to something like this, but was feeling like I was in over my head with the batfish helpers! Maybe this is something we could pair on next week?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pair!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per chat today, I got this wrong! You are already using the dataset; no need to offload to a batfish helper, but we maybe able to reduce some lines of code if we passed switchedNavigation
from PageLayout > Content > GroupedGuidesIndex.
Co-authored-by: Katy DeCorah <decorah@mapbox.com>
@@ -38,32 +38,47 @@ export default class PageLayout extends React.PureComponent { | |||
|
|||
// render the page's content | |||
renderContent = (config, parentPath, parent, hasSection) => { | |||
const { constants, frontMatter, location, domain } = this.props; | |||
const { constants, frontMatter, location, domain, navigation } = this.props; | |||
// If multi-structured show section name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: try using switchedNavigation
in place of navigation
here to minimize complexity of getSubPages
function in page layout utils.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to figure out how to use switchedNavigation
because it doesn't include hierarchy
, which is used getSubPages
. Let me know if I'm missing something!
* Rename NextPrevPage to NextPage; use RelatedPage in NextPage; add "next" option to RelatedPage * Typo * Replace "next" theme with "label" prop * update component name * update snapshot Co-authored-by: Colleen McGinnis <colleen.mcginnis@mapbox.com>
@@ -38,32 +38,47 @@ export default class PageLayout extends React.PureComponent { | |||
|
|||
// render the page's content | |||
renderContent = (config, parentPath, parent, hasSection) => { | |||
const { constants, frontMatter, location, domain } = this.props; | |||
const { constants, frontMatter, location, domain, navigation } = this.props; | |||
// If multi-structured show section name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to figure out how to use switchedNavigation
because it doesn't include hierarchy
, which is used getSubPages
. Let me know if I'm missing something!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks excellent, Colleen! Before you merge, I recommend creating a beta release and then testing the release in iOS or Android docs repository. This will allow you to test with real data and make updates to this PR if you find any bugs, instead of creating a release and then having to follow-up with a patch release in another PR.
Steps for a beta release:
- From this branch, manually update the version in package.json and package-lock.json (maybe
4.1.0-beta.0
). - Commit your changes.
- Run
npm run build
. - Then
cd pkg
andmbx npm publish
To test a new change committed to this branch, follow the same steps but increment the last number - .0
. So the next release from this branch 4.1.0-beta.1
.
Co-authored-by: Katy DeCorah <decorah@mapbox.com>
* main: Set NODE_ENV on `start`
* main: (43 commits) Handle NavigiationAccordion when there is no `activeItem` (#495) Do not preserve es modules in test environment (#498) 4.2.3 Update CHANGELOG.md Update CHANGELOG.md Fix DownloadButton for GeoJSON (#484) 4.2.2 Fix issue in `NavigationAccordion` to handle pages that do not have a parent (#479) 4.2.1 Update CHANGELOG.md 4.2.0 Prepare 4.2.0 Update CONTRIBUTING.md Update CONTRIBUTING.md Fix bug in `Feedback` textarea where the cursor jumped to end of text (#477) Add grouped guides functionality (#453) Add DownloadButton component (#474) 4.1.2 Only send feedback to Sentry if it exists (#475) 4.1.1 ...
Proposed approach
This PR proposes a way to group guides. @katydecorah can you take an initial look at this proposal that came out of last week's
The Write Stuff
session and let me know what you think of the direction?Contributor experience
The proposed approach works like this:
pages/({section}/)guides/
folder.index.md
file and its frontmatter should includegroup: true
.group: true
will automatically include aGuideGroupIndex
. This is a table of contents of all the guides in the group.groupOrder: {number}
in the frontmatter for each page.How existing functionality/components are affected
Several changes are necessary:
navigation
Batfish helper that's used to pass data to various components when building the site navigation.hierarchy
. Create a list of all the guide groups for reference. Then when iterating through the organized pages, if the page is a part of a guide group (hasgroupOrder
in the frontmatter), get the path of the group it belongs to and set that as the parent path. Then use that path to get the title of the group from the list of all guide groups you created earlier and set it as parent title.groupOrder
. Add the sorted list to a newsubPages
property for each group index page.group
andgroupOrder
frontmatter items to be included in the resultingnavigation
data. (I also addeddescription
so it can be used in theGuideGroupIndex
andNextPrevPage
components detailed below).NavigationAccordion
component to use this updated data including displaying an additional level of hierarchy.PageLayout
component to use updated hierarchy inBreadcrumbs
.New components with additional context
It also introduces two new components that accompany this change:
GuideGroupIndex
: This component creates a table of contents to add to thepage
layout when the frontmatter includesgroup: true
. This pulls in thetitle
anddescription
from the frontmatter of all pages in the same directory (group of guides). Looks like:NextPrevPage
: This component adds a next and previous link to the bottom of all pages with agroupOrder
in the frontmatter. This is meant to emphasize that these guides are meant to be read in the order. Looks like:How to test
TBD
QA checklist
#.#.#
.Open the test cases app locally on:
Before merge
npx browserslist@latest --update-db
to update the browser data and commit any changes to package-lock.json.cc @mapbox/docs @abhishek1508