fix(manifest): do not use nuxt loading bar color as default theme_color
#344
Conversation
set manifest theme-color to default to meta theme-color
maybebot
changed the title
docs/content/en/manifest.md
Outdated
| @@ -32,7 +32,7 @@ pwa: { | |||
| | `start_url` <sup>\*1</sup> | `String` | `routerBase + '?standalone=true'` | It has to be relative to where the manifest is placed | | |||
| | `display` <sup>\*1</sup> | `String` | `'standalone'` | | | |||
| | `background_color` <sup>\*2</sup> | `String` | `'#ffffff'` | | | |||
| | `theme_color` <sup>\*2</sup> | `String` | `this.options.loading.color` | Nuxt [loading color] option | | |||
| | `theme_color` <sup>\*2</sup> | `String` | `this.options.pwa.meta.theme_color` | This module's meta theme-color (see the [meta module]) | | |||
There was a problem hiding this comment.
I think mentioning this.options.* is not necessary as theme_color can be set same as rest in table. We only removed default
There was a problem hiding this comment.
Since these are the default options of the manifest, I thought it would be worth mentioning that it will have a default value from the PWA meta module's theme-color. If the docs say it's undefined but then has a value, even when not set, it might be confusing. What do you think?
There was a problem hiding this comment.
If the docs say it's undefined but then has a value, even when not set, it might be confusing. What do you think?
Yes this completely makes sense. We could just simplify content (see 4669426)
| @@ -27,15 +27,15 @@ function addManifest (pwa) { | |||
| start_url: routerBase + '?standalone=true', | |||
| display: 'standalone', | |||
| background_color: '#ffffff', | |||
| theme_color: this.options.loading && this.options.loading.color, | |||
| theme_color: this.options.pwa.meta.theme_color, | |||
| lang: 'en', | |||
| useWebmanifestExtension: false, | |||
| fileName: 'manifest.[hash].[ext]' | |||
| } | |||
There was a problem hiding this comment.
Maybe here we can delete theme_color property if it is falsy?
There was a problem hiding this comment.
JSON.stringify removes it when undefined for the manifestSource, but it is present in this.options.manifest as undefined. I can remove it with something like:
delete manifest.theme_color ? null : manifest.theme_color
or
if (!manifest.theme_color) { delete manifest.theme_color }
Sorry, I'm new to contributing to open source, and trying to get it right :) And thank you for the comments
pi0
changed the title
theme_color
Fixes issue #223
Removes the defaults for the meta
theme-colorand the manifesttheme-colortaken from the nuxt loading bar.To keep user configuration to a minimum, the manifest theme-color's default is now the meta one, if set.
In most real world scenarios they tend to coincide.
Docs updated to reflect the changes, plus a minor fix to the icon/meta links.