-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fix docs crash when navigating with hotkeys #6861
Conversation
Generate changelog in
|
} else if (isPageNode(node)) { | ||
this.routeToPage[node.route] = node.reference; | ||
} else if (parents[0] != null) { | ||
this.routeToPage[node.route] = parents[0].reference; |
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.
the !
was probably fine but no reason to not gracefully fail here I think rather than error out if we end up with something unexpected
fix nav issue with docs pageBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
if (isNavSection(node)) { | ||
return; |
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.
nit: Might be worth a comment that we never want to put the user on a "nav section" since these aren't click-able and don't have a real page. They're just presentational groupings of pages.
// HACKHACK: this is brittle | ||
// detect Components page and subheadings | ||
const COMPONENTS_PATTERN = /\/components(\.[\w-]+)?$/; | ||
const CONTEXT_PATTERN = /\/context(\.[\w-]+)?$/; | ||
const HOOKS_PATTERN = /\/hooks(\.[\w-]+)?$/; | ||
const LEGACY_PATTERN = /\/legacy(\.[\w-]+)?$/; | ||
export const isNavSection = ({ route }: HeadingNode) => | ||
COMPONENTS_PATTERN.test(route) || | ||
CONTEXT_PATTERN.test(route) || | ||
HOOKS_PATTERN.test(route) || | ||
LEGACY_PATTERN.test(route); |
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.
Moving this from docs-app
to docs-theme
feels a bit strange. I think docs-theme
is meant to be generalizable outside of the Blueprint repo, and these regex patterns look specific to Blueprint's docs-app
.
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.
ah I didn't understand what the distinction was for, I suppose some other project could rely on docs-theme to use the blueprint docs styling and provide their own content?
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.
Yup! I think that's the intention. I'm not sure how many other projects actually do that though.
this.routeToPage[node.route] = reference; | ||
if (isNavSection(node)) { | ||
return; | ||
} else if (isPageNode(node)) { |
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.
Is the "components" navigation section passing the isPageNode
check? Could that be a bug in Documentalist or are we setting up the components section in a weird/unexpected way?
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.
going on intuition haven't dug into the code much here but I believe "components" is a page node because it has sub-pages - /docs/#core/components/breadcrumbs
const navItemElement = this.navElement.querySelector<HTMLElement>(`a[href="#${activeSectionId}"]`)!; | ||
const navItemElement = this.navElement.querySelector<HTMLElement>(`a[href="#${activeSectionId}"]`); | ||
if (navItemElement == null) { | ||
return; |
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.
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.
Agree!
diffBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
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.
Thanks! 🥳
const { reference } = isPageNode(node) ? node : parents[0]!; | ||
this.routeToPage[node.route] = reference; | ||
if (isPageNode(node)) { | ||
if (this.props.navigatorExclude?.(node)) { |
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.
Oh nice! I see we were already setting this prop.
const navItemElement = this.navElement.querySelector<HTMLElement>(`a[href="#${activeSectionId}"]`)!; | ||
const navItemElement = this.navElement.querySelector<HTMLElement>(`a[href="#${activeSectionId}"]`); | ||
if (navItemElement == null) { | ||
return; |
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.
Agree!
Fixes #6381
Checklist
Changes proposed in this pull request:
Ignore non-navigational headers so that navigating with hotkeys does not crash the page, and users cannot get to a page with no useful info.
Reviewers should focus on:
Also considered
Allowing these headers to be navigated to, but you end up on a useless page
Screenshot
Current:
This PR: