-
Notifications
You must be signed in to change notification settings - Fork 91
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
[WNMGDS-3243] Upgraded Gatsby to v5 #3477
[WNMGDS-3243] Upgraded Gatsby to v5 #3477
Conversation
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 next set of markdown file changes have to do with preventing Interleaving
@@ -4,10 +4,29 @@ order: 70 | |||
intro: System color tokens are the entire set of color tokens from which our themes are built. | |||
--- | |||
|
|||
import ColorContrastGuidelines from '../../src/reusable-text-snippets/ColorContrastGuidelines.mdx'; |
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 recreate this functionality in Gatsby v5, so instead of using the reusable text snippet, I've gone ahead and copied and pasted the contents into this markdown file, plus the other three locations where the content was changed. Another option would be to create a react component and import it accordingly.
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 think the React component approach makes the most sense. Do you agree?
If so, we can spin up a ticket for that work.
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.
@jack-ryan-nava-pbc agreed! New ticket -> https://jira.cms.gov/browse/WNMGDS-3287
<PrintIcon ariaHidden={false} className="ds-u-margin-right--1" /> | ||
</Button> | ||
<Button variation="ghost" size="small"><PrintIcon ariaHidden={false} className="ds-u-margin-right--1" />Print</Button> |
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.
These changes prevent interleaving, which affects the generated output and visual display
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, (I think) it's possible that someone could have auto formatting enabled in their IDE that wraps content to new lines on save or something. We should spin up a new confluence page documenting the nuts and bolts of the Doc Site so we can refer to why we need these things on one line and other miscellany.
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.
@jack-ryan-nava-pbc that's a good call! The latest commit (de66bb7) runs the repository's prettier settings against the markdown file changes
export const onCreateNode = ({ node, getNode, actions }) => { | ||
const { createNodeField } = actions; | ||
if (node.internal.type === `Mdx`) { | ||
const slug = createFilePath({ node, getNode, basePath: `pages`, trailingSlash: false }); |
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.
We no longer get slug creation for free and need to apply it to each note
title | ||
} | ||
fields { | ||
slug |
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.
slug is no longer "top-level"
@@ -55,7 +54,10 @@ const CodeWithSyntaxHighlighting = ({ | |||
// for preformatted text that has code as it's child, set language class on <pre> too | |||
// this allows scrolling in code block on small screens | |||
const PreformattedWithLanguageClass = (props: any) => { | |||
if (props.children?.props?.mdxType === 'code' && props.children?.props?.className) { | |||
if ( | |||
props.children?.type?.name === 'CodeWithSyntaxHighlighting' && |
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 type changed and ensures styling for code blocks is unchanged
@@ -1,7 +1,6 @@ | |||
import Prism from 'prismjs'; | |||
import { ThirdPartyExternalLink } from '@cmsgov/design-system'; | |||
|
|||
import { MDXRenderer } from 'gatsby-plugin-mdx'; |
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.
MDXRenderer
is no longer available as part of gatsby-plugin-mdx
at version 2 and higher
|
||
createPage({ | ||
path: originalPage.path, | ||
component: `${originalPage.component}?${contentFilePath}`, |
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 component
argument ensures that the template .tsx file is "linked" to the content that is defined by contentFilePath
. The "gatsby-plugin-mdx" plugin will then convert the markdown to HTML/CSS/JS that can be accessed with the children
prop within the template component.
}, | ||
} = edge; | ||
// Path for this page -- the slug with positioning markers removed | ||
const path = edge.node.fields.slug.replace(/\d+_/g, ''); |
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 there ever a chance a slug might be missing, and should we check for it before processing? My assumption is no, since we create slugs for each Mdx node in onCreateNode, which presumably runs before this. Just checking my understanding of how these steps fit together.
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.
@tamara-corbalt good question!
It's possible to explore the created nodes using the GraphiQL endpoint (http://localhost:8000/___graphql) when running npm run start
and verify that the slugs and titles exist using this query.
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.
All the slugs were created as intended as part of the earlier step onCreateNode
(docs api reference)!
@@ -12,8 +12,9 @@ import { Alert } from '@cmsgov/design-system'; | |||
<ThemeContent neverThemes={['medicare']}> | |||
<Alert variation="error"> | |||
<p class="ds-c-alert__text"> | |||
This component is only used for Medicare. Please use the theme switcher to view the component | |||
with Medicare styles. | |||
{ |
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.
Ok, so the brackets prevent a second <p>
tag from being interleaved within the other <p>
tag?
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.
Yes, that's correct
@@ -180,19 +180,19 @@ Align columns horizontally or vertically using [flexbox utility classes](/utilit | |||
</div> | |||
<div | |||
class="ds-l-row ds-u-align-items--start ds-u-border--1 ds-u-padding--1 ds-u-margin-y--2" | |||
style="height: 100px" | |||
style={{ height: 100 }} |
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 lack of units here makes me nervous. PX is the default?
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.
Yes, PX is the default
{' '} | ||
.ds-u-display--inline-flex{' '} | ||
</code> | ||
<code class="ds-u-display--block ds-u-padding--1 ds-u-margin-bottom--2 utility-example"> .ds-u-display--block </code> |
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 so much better than the interpolated spaces. Thank you!
@@ -72,7 +72,7 @@ export const TableOfContentsFeedback = ({ slug }: TableOfContentsFeedbackProps) | |||
<li> | |||
<a | |||
onClick={linkAnalytics} | |||
href={`https://github.com/CMSgov/design-system/edit/main/packages/docs/content/${slug}.mdx`} | |||
href={`https://github.com/CMSgov/design-system/edit/main/packages/docs/content${slug}.mdx`} |
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 / in the slug itself or is content not a folder anymore?
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 /
is in the slug now
sendViewEvent(analyticsPayload); | ||
} | ||
}, []); | ||
import useSendViewEvent from '../helpers/useSendViewEvent'; |
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! Thank you for extracting this.
@@ -0,0 +1,182 @@ | |||
import express from 'express'; |
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'd love (another) walkthrough of these changes!
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 think I need a walkthrough of the gatsby-node file changes, but pretty much everything else looks a-ok!
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.
Walked through changes in call on 3/19/2025 Looks good!
Summary
<p>
tag around new lines with something called interleavingHow to test
npm install
npm run start
Checklist
[WNMGDS-####] Title
or [NO-TICKET] if this is unticketed work.Type
(only one) label for this PR, if it is a breaking change, label should only beType: Breaking
Impacts
, multiple can be selected.