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

chore(starters): Migrate starters to Head API #36234

Merged
merged 13 commits into from
Jul 27, 2022

Conversation

marvinjude
Copy link
Contributor

Migrate Starters to Head API

[sc-53425]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 25, 2022
@marvinjude marvinjude added topic: DX Developer Experience (e.g. Fast Refresh, i18n, SSR, page creation, starters) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jul 25, 2022
@marvinjude marvinjude changed the title chore(starters): Migrate starters to head api chore(starters): Migrate starters to Head api Jul 25, 2022
@marvinjude marvinjude changed the title chore(starters): Migrate starters to Head api chore(starters): Migrate starters to Head API Jul 25, 2022
Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

You should run prettier on every starter, the formatting feels off here and there (weird spaces between brackets for example)

You'll also need to update all package-lock.json files

starters/blog/src/components/seo.js Outdated Show resolved Hide resolved
starters/blog/src/pages/using-typescript.tsx Outdated Show resolved Hide resolved
starters/default/src/components/seo.js Outdated Show resolved Hide resolved
starters/blog/src/components/seo.js Show resolved Hide resolved
starters/blog/src/templates/blog-post.js Outdated Show resolved Hide resolved
starters/default/src/pages/using-typescript.tsx Outdated Show resolved Hide resolved
@marvinjude marvinjude requested a review from LekoArts July 26, 2022 13:17
@@ -65,6 +63,8 @@ const BlogIndex = ({ data, location }) => {

export default BlogIndex

export const Head = () => <Seo title="All posts" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Really nice and simple refactor / API here @marvinjude. Nicely done.

@@ -4,4 +4,6 @@
* See: https://www.gatsbyjs.com/docs/ssr-apis/
*/

// You can delete this file if you're not using it
exports.onRenderBody = ({ setHtmlAttributes }) => {
setHtmlAttributes({ lang: `en` })
Copy link
Contributor

Choose a reason for hiding this comment

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

Should or have we documented this? (I've typically myself set the html attributes with the Helmet component, so it's possible that's still documented somewhere!)

The context: it's a reasonably common use case for folks, and if I recall, it's one thing that Lighthouse recommends too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

The starters_validate test fails. Can you look into that?

@marvinjude
Copy link
Contributor Author

marvinjude commented Jul 26, 2022

The starters_validate test fails. Can you look into that?

Yep! It's all good now

@LekoArts LekoArts merged commit cc548c0 into master Jul 27, 2022
@LekoArts LekoArts deleted the migrate-starters-to-head-api branch July 27, 2022 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: DX Developer Experience (e.g. Fast Refresh, i18n, SSR, page creation, starters)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants