[gatsby-plugin-manifest] Add option to remove theme-color meta tag

[GriffinJohnston]

Summary

gatsby-plugin-manifest inserts a theme-color meta tag into the site head. This means that you can't use React Helmet to set the theme_color, which is a problem for sites with a dark mode/theme-switcher/etc that want to programmatically change color schemes.

We could either remove the meta tag insertion entirely, since it's redundant with the theme_color value in the manifest, or — if the meta tag is required for some corner-cases I'm not aware of — add an option to turn off this behavior.

I'm just diving into Gatsby, and I've been super impressed so far. Thanks everyone for the hard work on this awesome project!

[DSchau]

This would be a great first PR! Would you be interested in making the changes?

Whether you are or not, I can walk through how someone would make the changes.

Feature Request: Allow disabling of theme-color from meta-tag

In some scenarios, it may make sense to disable the automatic meta theme-color injection, i.e. in scenarios where the application is themed and the theme-color tag will need to update.

What's involved

• Adding an option disableThemeColor (feel free to tweak this) which is defaulted to true
• Detecting this condition, and if false, not adding the meta tag

Technical Detail

1. Add some default options here
2. Detect condition (if false) and conditionally do not add if false here
   • Note, Array.prototype.concat is helpful here, e.g. ['red'].concat(true ? ['blue'] : []).concat(false ? ['green'] : []) will create ['red', 'blue']
3. Add some unit tests validating the functionality (note: jest.fn() is super helpful here, e.g. a spy!)

[GriffinJohnston]

Thanks for the quick reply! I'll put together a PR when I get a moment.

[DZuz14]

Didn't mean to steal your thunder here @GriffinJohnston, I can rescind this PR if you were indeed currently working on this. Just thought it might be nice to bang it out and get on to the next one!

[GriffinJohnston]

@DZuz14 Not at all! I got really busy soon after posting this and never got around to it. Glad you jumped on it!

Edit: actually your PR doesn't address my issue! What I want is an option that still allows setting a theme_color (for the webmanifest) but doesn't output a theme color meta tag in the head of the site. Should be able to get to this over the weekend.

[pieh]

There should be a way to make react-helmet recognize our tag - if we add data-react-helmet="true to the tag. Then react-helmet will be able to update it (and we won't need extra config option for this)

[DZuz14]

Haha whoops. So did my PR really accomplish anything?

[GriffinJohnston]

@pieh I was considering that, but there are plans to remove the use of data-react-helmet="true" from react-helmet, see nfl/react-helmet#79

@DZuz14 Not for my purposes! Haha. I wasn't sure if you were addressing a separate problem you had run into.

[pieh]

Haha whoops. So did my PR really accomplish anything?

That was still good sanity change - if theme-color is not defined we shouldn't create meta tag with value of undefined ;)

@pieh I was considering that, but there are plans to remove the use of data-react-helmet="true" from react-helmet, see nfl/react-helmet#79

Yeah I saw this last year. It will be great once it's done, but it's not done yet.

I feel like it would be easier for users to not care about that extra config option - problem with this is that they need to know about it - so scenario would be - they notice this problem of duplicated theme-color meta tag and start searching for solution and through issues or docs see that config option. When applying (at least temporarily that react-helmet attribute), they wouldn't even hit that issue - so from DX perspective this seem like nicer way to do it? But I don't have strong opinion about this, I'm fine with creating option to disable it - only thing would be to expand condition for adding meta tag and delete option key here https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-manifest/src/gatsby-node.js#L25-L27 so it doesn't end up in manifest file

[GriffinJohnston]

I agree that adding data-react-helmet="true" is a lot more elegant, but I worry about adding code that we know will be obsolete at some point in the near to medium term? Is there a system for flagging this kind of thing to make sure it gets updated once that change happens?

[DZuz14]

Ah, I see the problem that you are having now. I would agree with @GriffinJohnston on this one. As long as this option was to be well documented with an explanation of why it's useful in some cases, I think we should have trust in the user to be resourceful enough to check the documentation if they do indeed experience any duplications. At least you will be future proofing the plugin to not depend on third party code that could change.

[DZuz14]

Maybe the documentation for the plugin could prominently display a quick warning/tip for users who dynamically set theme colors in their app, with a link to that config option with a code example or something.

[ivorpad]

@pieh I can try fixing this issue as my first Gatsby PR if you don't mind. What would be the best way to pass in_head as an option?

I was thinking about:

theme_color: {
  color: '#663399',
  in_head: false
}

but I decided against it because pluginOptions.theme_color.color is redundant.

Maybe:

theme_color_in_head: false (?)

I'd gladly set some of my time to get this done if you want/need.

[ivorpad]

@pieh My PR above fixes @GriffinJohnston's issue.

I'm open to discussion & advice.

[pieh]

@ivorpad that's great!

We definitely need to leave theme_color option as is, because it gets written to manifest file ( https://developer.mozilla.org/en-US/docs/Web/Manifest#theme_color ), so using object would require some extra unnecessary code to normalize it.

As for naming the option - that's always the hard part ;) theme_color_in_head looks pretty weird (with underscored), but it follows conventions from manifest and does convey what this option does so I'm fine with this. @GriffinJohnston any thoughts on this?

[GriffinJohnston]

That name works! We could also use something like theme_color_meta which is a touch shorter. This was always going to have an awkward name I think 😆

[ivorpad]

I'm the king of weird names for variables 😂theme_color_in_head was the first name that came to my mind.

Open to suggestions, though.