-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(gatsby-plugin-manifest): don't output theme-color
if it's not defiened
#10069
feat(gatsby-plugin-manifest): don't output theme-color
if it's not defiened
#10069
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.
This looks reasonable - maybe small code change so it's easier to read what it does.
Another thing - can you remove changes in packages/gatsby-source-faker/README.md
?
] | ||
: [] | ||
) | ||
) |
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 prep headComponents array ahead of time to avoid concat with ternary operator inside it?
const headComponents = [
<link
key={`gatsby-plugin-manifest-link`}
rel="manifest"
href={withPrefix(`/manifest.webmanifest`)}
/>,
]
if (pluginOptions.theme_color) {
headComponents.push(
<meta
key={`gatsby-plugin-manifest-meta`}
name="theme-color"
content={pluginOptions.theme_color}
/>
)
}
setHeadComponents(headComponents)
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.
Yeah, I thought I got rid of those changes to that ReadMe. I am not even sure how they got changed to be honest. Dog stepping on keyboard perhaps lol.
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 it alright that I created the headComponents array at the top of the function, and changed the other conditionals to just add to this array as we step through the function? I think this makes it pretty legible and easy to understand whats going on. I ran the tests after this change and everything still seems to work just fine.
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, yeah, that's fine too 👍
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.
Hmm, actually this README changes (that your dog committed 🤣 ) seems like something very weird is happening and I get them locally too, I'll just merge with those - hopefully this will fix problems lol
theme-color
if it's not defiened
Holy buckets, @DZuz14 — we just merged your PR to Gatsby! 💪💜 Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! |
published |
…it's not defiened (gatsbyjs#10069) Not really sure how I go about submitting a PR here. This addition references issue gatsbyjs#9977 Any guidance would be greatly appreciated.
Not really sure how I go about submitting a PR here. This addition references issue #9977
Any guidance would be greatly appreciated.