Always set theme color to background color ✅ #19391
Conversation
|
@newmuis PTAL |
| let meta = document.createElement('meta'); | ||
| const styles = getComputedStyle(document.body); | ||
| meta.name = "theme-color"; | ||
| meta.content = styles.getPropertyValue('primary-color') || '#F1F3F4'; |
There was a problem hiding this comment.
nit: can you extract the #F1F3F4 to a constant (e.g. DEFAULT_THEME_COLOR) declared at the top of this file?
| // if possible, with the fall back being default light gray. | ||
| let meta = document.createElement('meta'); | ||
| const styles = getComputedStyle(document.body); | ||
| meta.name = "theme-color"; |
There was a problem hiding this comment.
nit: Use single quotes for string values
| setThemeColor_(){ | ||
| // The theme color should be copied from the story's primary accent color | ||
| // if possible, with the fall back being default light gray. | ||
| let meta = document.createElement('meta'); |
There was a problem hiding this comment.
So, AMP is very weird about the way that we use document. If possible, you want to get it from the window object (i.e. this.win.document), because IIRC in some cases, there could be more than one window, and/or more than one document.
| meta.name = 'theme-color'; | ||
| meta.content = styles.getPropertyValue('primary-color') || | ||
| DEFAULT_THEME_COLOR; | ||
| document.getElementsByTagName('head')[0].appendChild(meta); |
There was a problem hiding this comment.
I think you want to apply what you used to get the document (this.win.document) to the other document statements :)
| /** | ||
| * @private | ||
| */ | ||
| setThemeColor_() { |
There was a problem hiding this comment.
It is very unlikely, but publishers may provide a <meta name="theme-color" ... > when writing their story HTML. Should we try to check if there's one, so we don't override it?
We could check this by doing something like
if (this.win.document.querySelector('meta[name=theme-color]')) {
return;
}
What do you think about this?
There was a problem hiding this comment.
I think this makes a lot of sense! Will add in a check.
There was a problem hiding this comment.
Not that unlikely. E.g. the PWA audit in Lighthouse recommends it.
| story.buildCallback(); | ||
| const metaTags = story.element.getElementsByTagName('meta'); | ||
| const metaTagNames = Array.from(metTagNames).map(el => el.name); | ||
| expect(metaTagNames.contains('theme-color')).to.be.true; |
There was a problem hiding this comment.
Nit: you could avoid looping through an array by selecting your meta tag directly. Something like
const metaTag = win.document.querSelector('meta[name=theme-color]');
| const styles = computedStyle(this.win.document.body); | ||
| meta.name = 'theme-color'; | ||
| meta.content = styles.getPropertyValue('primary-color') || | ||
| DEFAULT_THEME_COLOR; |
There was a problem hiding this comment.
nit: I think the style is to indent hanging lines +4 spaces from the previous line. So:
meta.content = styles.getPropertyValue('primary-color') ||
DEFAULT_THEME_COLOR;
| /** | ||
| * @private | ||
| */ | ||
| setThemeColor_() { |
There was a problem hiding this comment.
Not that unlikely. E.g. the PWA audit in Lighthouse recommends it.
| meta.name = 'theme-color'; | ||
| meta.content = styles.getPropertyValue('--primary-color') || | ||
| DEFAULT_THEME_COLOR; | ||
| this.win.document.getElementsByTagName('head')[0].appendChild(meta); |
* Added theme-color based on accent color for ampstory * Added theme-color based on accent color for ampstory * Updated JSDoc for method * fixed lint errors and test errors * fixed suggestions from code review * use computed style helper instead of getComputedStyle * fixed tests, implemented code review suggestions
Closes #19203
Added theme-color meta-tags based on the primary accent color of the story. Should the primary accent color not exist, the default theme-color is set to light gray with black text (which is the default for lighter theme colors).