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

TS - templates migration #6479

Merged
merged 1 commit into from
Jun 2, 2022
Merged

TS - templates migration #6479

merged 1 commit into from
Jun 2, 2022

Conversation

pettinarip
Copy link
Member

Description

Migration to TS of the templates.

Related Issue

#6392

@github-actions github-actions bot added dependencies 📦 Changes related to project dependencies review needed 👀 labels May 26, 2022
@gatsby-cloud
Copy link

gatsby-cloud bot commented May 26, 2022

Gatsby Cloud Build Report

ethereum-org-website-dev

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 27m

Performance

Lighthouse report

Metric Score
Performance 🔶 21
Accessibility 💚 100
Best Practices 💚 100
SEO 💚 92

🔗 View full report

@@ -165,15 +165,22 @@ const Contributors = styled(FileContributors)`
}
`

const DocsPage = ({ data, pageContext }) => {
const DocsPage = ({
data: { siteData, pageData: mdx },
Copy link
Member

Choose a reason for hiding this comment

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

Not specific to this PR but just noting - should we just update pageData to mdx in the query? Seems kind of silly to rename the variable here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, would make sense. Looks like we immediately rename is in all cases except for the Tutorials template, which is now converted to mdx in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. This was a try to make them consistent but doing them at the query level is better.


const tocItems = mdx.tableOfContents.items
if (!siteData || !mdx?.frontmatter) {
throw new Error("Docs page template query does not return expected values")
Copy link
Member

Choose a reason for hiding this comment

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

👍

const tocItems = mdx.tableOfContents.items
const { summaryPoints } = mdx.frontmatter
const StakingPage = ({
data: { pageData: mdx },
Copy link
Member

Choose a reason for hiding this comment

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

Same here - should we just rename pageData to mdx in the query? 😄


const isRightToLeft = isLangRightToLeft(mdx.frontmatter.lang as Lang)

// FIXME: remove this any, currently not sure how to fix the ts error
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm what was the TS error? Is the type inconsistent?

Not sure if the comment below this is related:

// TODO some `gitLogLatestDate` are `null` - why?

Copy link
Member Author

Choose a reason for hiding this comment

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

So, the auto-generated types for mdx are kind of weird. I have an error with in the next line where we call parent.fields.

The generated type for mdx is saying that it could possibly be {}, not sure if that is correct.

The error:

Property 'fields' does not exist on type '{} | { readonly mtime: string; readonly fields: { readonly gitLogLatestDate: string | null; } | null; }'.
  Property 'fields' does not exist on type '{}'.

const pageData = data.pageData
const isRightToLeft = isLangRightToLeft(pageData.frontmatter.lang)
const TutorialPage = ({
data: { siteData, pageData: mdx },
Copy link
Member

Choose a reason for hiding this comment

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

Same here 😄

Copy link
Member

@samajammin samajammin left a comment

Choose a reason for hiding this comment

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

Nice! LGTM. Made a few comments but those are unrelated to your PR.

@wackerow wackerow merged commit f0b0b0b into dev Jun 2, 2022
@wackerow wackerow deleted the ts-templates branch June 2, 2022 23:40
@wackerow wackerow mentioned this pull request Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies 📦 Changes related to project dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants