-
Notifications
You must be signed in to change notification settings - Fork 36
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
DOP-5078: Fix duplicate "docs" prefix in BreadcrumbList #1285
Conversation
✅ Deploy Preview for mongodb-snooty ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…repeated-docs-breadcrumbs
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.
nice catch! mostly LGTM. just two non blocking comments
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.
can we also add a test with breadcrumbs representative of the db. these contain paths that are relative ie.
{
"baseUrl": "https://mongodb.com/docs/",
"slug": "atlas/cli/stable/",
"title": "Atlas CLI",
"projectName": "atlas-cli",
"showInProductsList": true,
"breadcrumbs": [
{
"title": "MongoDB Atlas",
"path": "/atlas"
}
]
}
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's an existing test that includes intermediate breadcrumbs with full URLs as well as breadcrumbs with relative paths. Is that what you're looking for?
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 think this was not tested as this is used in DocumentBody and not caught in a snapshot (as far as Structured Data)
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.
Ahhh, I can add a separate test for the structured data as part of this PR on Monday
@@ -12,6 +12,7 @@ export const useSiteMetadata = () => { | |||
parserUser | |||
patchId | |||
pathPrefix | |||
project |
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.
Trying to figure out why this was added in this PR
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.
When checking for usage of siteUrl
, I noticed that the build metadata object we use for this hook had project
, but the hook was not returning project
. I added it here for completeness so it can be used properly.
It's completely out of scope for this PR, but added it as a bonus since it was a small change. Happy to create a separate PR for it if you'd prefer!
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.
Keep it in! Thank you for the explanation!
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.
changes LGTM. can follow up to include a test of breadcrumb structured data
@@ -22,10 +22,6 @@ const FootnoteReference = ({ nodeData: { id, refname } }) => { | |||
const { footnotes } = useContext(FootnoteContext); | |||
const { darkMode } = useDarkMode(); | |||
|
|||
// the nodeData originates from docutils, and may be incorrect for |
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.
guessing this change is not from this PR?
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.
It's one of the bonus changes mentioned above. Figured I'd just lump it in this PR since it's small and I was planning on making a commit for it anyways, but happy to make it separate if needed!
…repeated-docs-breadcrumbs
@seungpark I added tests in the latest commits. Please feel free to take a look |
…repeated-docs-breadcrumbs
Stories/Links:
DOP-5078
Current Behavior:
Staging Links:
Notes:
baseUrl()
included/docs/
after the site URL. This meant that whenwithPrefix
was applied while constructing breadcrumbs, there'd be duplicate prefixes in the final full URL.Bonus
project
to the site metadata gql hook for completeness.README updates