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

fix(starters): Improve null checking & update to best practices #26918

Merged
merged 10 commits into from
Sep 17, 2020

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Sep 16, 2020

Description

In order to reduce friction when people modify the default and blog starters I improved those two a bit. Both with newer best practices but also safe-guarding against null by using optional chaining. Furthermore a bit of schema customization for blog was added.

  • Changed .org to .com links
  • Changes edges { node } to nodes
  • Checked for existence of images, titles, siteMetadata items and set default values
  • Added schema customization for markdown type
  • Changed all caps NOT FOUND to something better for screen readers

Notes

In theory we could also define the default values for e.g. siteMetadata items in the schema customization but I think JS code is more discover-able and easier to understand.

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 16, 2020
@LekoArts LekoArts removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 16, 2020
laurieontech
laurieontech previously approved these changes Sep 16, 2020
Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

Yes please! Thanks for doing this Lennart.

@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 16, 2020

Gatsby Cloud Build Report

gatsby

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

Build Details

View the build logs here.

🕐 Build time: 19m

ascorbic
ascorbic previously approved these changes Sep 16, 2020
Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Great work!

starters/blog/gatsby-node.js Outdated Show resolved Hide resolved
previous,
next,
},

Copy link
Contributor

Choose a reason for hiding this comment

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

This check shouldn't be needed, because if the array is empty, the forEach never happens anyway. However, what might be needed is a null check on allMarkdownRemark. Does that exist if there's no markdown? Shame there's no optional chaining in gatsby-node!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to make it more explicit as I myself can't always remember that e.g. forEach would behave this way


// Also explicitly define the Markdown frontmatter
// This way the "MarkdownRemark" queries will return `null` even when no
// blog posts are stored inside "content/blog" instead of returning an error
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, well that answers my question. Does it need a null check or does it have an empty nodes array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an empty array then :)

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The markdownRemark query returns null

<Layout location={location} title={siteTitle}>
<SEO title="All posts" />
<Bio />
<p>No blog posts found. Add markdown posts to "content/blog" (or the directory you specified for the "gatsby-source-filesystem" plugin in gatsby-config.js).</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Big fan of this approach. Nice work.

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

I love this so much, thank you for tackling this! Less friction, fewer papercuts, easier learning by doing 👏

Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

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

Once again, great work on this.

@LekoArts LekoArts merged commit 6243ca4 into master Sep 17, 2020
@LekoArts LekoArts deleted the robust-starters branch September 17, 2020 08:49
@fk
Copy link
Contributor

fk commented Sep 17, 2020

<3

Comment on lines +70 to +71
<a href={`https://twitter.com/${social?.twitter || ``}`}>
You should follow them on Twitter
Copy link
Contributor

Choose a reason for hiding this comment

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

i think, if there is no twitter name is set, then this a tag shot not printed?

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants