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.

Sign up for GitHub

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

Jump to bottom

Always set theme color to background color ✅ #19391

Merged
merged 8 commits into from Nov 20, 2018
Merged

Always set theme color to background color ✅ #19391

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
37bc45f
Added theme-color based on accent color for ampstory
bramanudom Nov 19, 2018
99fe621
Added theme-color based on accent color for ampstory
bramanudom Nov 19, 2018
4290304
Merge branch 'master' into theme_color
bramanudom Nov 19, 2018
dfc75da
Updated JSDoc for method
bramanudom Nov 19, 2018
42b6ba1
fixed lint errors and test errors
bramanudom Nov 19, 2018
57158a3
fixed suggestions from code review
bramanudom Nov 20, 2018
9effbcd
use computed style helper instead of getComputedStyle
bramanudom Nov 20, 2018
8a13ee5
fixed tests, implemented code review suggestions
bramanudom Nov 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions extensions/amp-story/1.0/amp-story.js
View file
Open in desktop
Expand Up @@ -180,6 +180,11 @@ const TAG = 'amp-story';
const HIDE_ON_BOOKEND_SELECTOR =
'amp-story-page, .i-amphtml-story-system-layer';

/**
* The default light gray for chrome supported theme color.
* @const {string}
*/
const DEFAULT_THEME_COLOR = '#F1F3F4';

/**
* @implements {./media-pool.MediaPoolRoot}
Expand Down Expand Up @@ -348,6 +353,7 @@ export class AmpStory extends AMP.BaseElement {
this.initializeListeners_();
this.initializeListenersForDev_();

this.setThemeColor_();
this.storeService_.dispatch(Action.TOGGLE_UI, this.getUIType_());

this.navigationState_.observe(stateChangeEvent => {
Expand Down Expand Up @@ -418,6 +424,20 @@ export class AmpStory extends AMP.BaseElement {
});
}

/**
* @private
*/
setThemeColor_() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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 think this makes a lot of sense! Will add in a check.

Copy link
Member

Choose a reason for hiding this comment

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

Not that unlikely. E.g. the PWA audit in Lighthouse recommends it.

// 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');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const styles = computedStyle(this.win.document.body);
meta.name = 'theme-color';
meta.content = styles.getPropertyValue('primary-color') ||
DEFAULT_THEME_COLOR;
Copy link
Contributor

Choose a reason for hiding this comment

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

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;

document.getElementsByTagName('head')[0].appendChild(meta);
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 you want to apply what you used to get the document (this.win.document) to the other document statements :)

}

/**
* @private
*/
Expand Down
8 changes: 8 additions & 0 deletions extensions/amp-story/1.0/test/test-amp-story.js
View file
Open in desktop
Expand Up @@ -398,6 +398,14 @@ describes.realWin('amp-story', {
});
});

it('should have a meta tag that sets the theme color', () => {
createPages(story.element, 2);
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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]');

});

describe('amp-story consent', () => {
it('should pause the story if there is a consent', () => {
sandbox.stub(Services, 'actionServiceForDoc')
Expand Down