-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃寜New "mobile" view for ads in fullbleed story #26243
Conversation
@@ -127,6 +139,9 @@ export class StoryAdPage { | |||
|
|||
/** @private {boolean} */ | |||
this.viewed_ = false; | |||
|
|||
/** @private @const {!../../amp-story/0.1/amp-story-store-service.AmpStoryStoreService|!../../amp-story/1.0/amp-story-store-service.AmpStoryStoreService} */ |
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 desirable to have inter-version dependency?
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.
Good point! We should clean it but that'd be on the AMP Story WG to do it when we delete all the 0.1
code
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.
SGTM.
@@ -48,6 +53,9 @@ const TIMEOUT_LIMIT = 10000; // 10 seconds | |||
/** @const {string} */ | |||
const GLASS_PANE_CLASS = 'i-amphtml-glass-pane'; | |||
|
|||
/** @const {string} */ | |||
const DESKTOP_FULLBLEED_CLASS = 'i-amphtml-story-desktop-fullbleed'; |
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.
Should we add another prefix to make it different from the class the AMP Story runtime uses? I'm afraid of CSS properties collisions eventually
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.
Good point, done.
|
||
if (uiState === UIType.DESKTOP_FULLBLEED) { | ||
this.adChoicesIcon_.classList.add(DESKTOP_FULLBLEED_CLASS); | ||
} |
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.
If you feel like factorizing this in a single line (it's a bit less clear tho, so up to you)
this.adChoicesIcon_.classList.toggle(
DESKTOP_FULLBLEED_CLASS, uiState === UIType.DESKTOP_FULLBLEED);
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.
I like it :) Done.
Story ads so far have not been responsive. Introduces a new view for fullbleed stories on desktop where we render the ad in a mobile like size.