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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃悰 Move ampDoc references into amp-story buildCallback #30986

Merged
merged 5 commits into from Nov 4, 2020
Merged

馃悰 Move ampDoc references into amp-story buildCallback #30986

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2fb1618
Move methods that need ampdoc to buildCallback.
processprocess Nov 3, 2020
17e797e
Update types.
processprocess Nov 3, 2020
498942d
Remove const
processprocess Nov 3, 2020
bd58585
Fix typo.
processprocess Nov 3, 2020
0bd09a2
devAssert.
processprocess Nov 4, 2020
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
28 changes: 17 additions & 11 deletions extensions/amp-story/1.0/amp-story.js
View file
Open in desktop
Expand Up @@ -327,13 +327,14 @@ export class AmpStory extends AMP.BaseElement {
/** @private @const {!../../../src/service/platform-impl.Platform} */
this.platform_ = Services.platformFor(this.win);

/** @private @const {!../../../src/service/viewer-interface.ViewerInterface} */
this.viewer_ = Services.viewerForDoc(this.element);
/** @private {?../../../src/service/viewer-interface.ViewerInterface} */
this.viewer_ = null

/** @private @const {?AmpStoryViewerMessagingHandler} */
this.viewerMessagingHandler_ = this.viewer_.isEmbedded()
? new AmpStoryViewerMessagingHandler(this.win, this.viewer_)
: null;
/** @private {?AmpStoryViewerMessagingHandler} */
this.viewerMessagingHandler_ = null

/** @private {?../../../src/service/localization.LocalizationService} */
this.localizationService_ = null;

/**
* Store the current paused state, to make sure the story does not play on
Expand All @@ -353,8 +354,16 @@ export class AmpStory extends AMP.BaseElement {

/** @private {?LiveStoryManager} */
this.liveStoryManager_ = null;
}

/** @override */
buildCallback() {
this.viewer_ = Services.viewerForDoc(this.element);

this.viewerMessagingHandler_ = this.viewer_.isEmbedded()
? new AmpStoryViewerMessagingHandler(this.win, this.viewer_)
: null;

/** @private @const {!../../../src/service/localization.LocalizationService} */
this.localizationService_ = getLocalizationService(this.element);

this.localizationService_
Expand Down Expand Up @@ -389,10 +398,7 @@ export class AmpStory extends AMP.BaseElement {
'en-xa',
enXaPseudoLocaleBundle
);
}

/** @override */
buildCallback() {
if (this.isStandalone_()) {
this.initializeStandaloneStory_();
}
Expand Down Expand Up @@ -1003,7 +1009,7 @@ export class AmpStory extends AMP.BaseElement {
// Preloads and prerenders the share menu.
this.shareMenu_.build();

const infoDialog = shouldShowStoryUrlInfo(this.viewer_)
const infoDialog = this.viewer_ && shouldShowStoryUrlInfo(this.viewer_)
Copy link
Contributor

Choose a reason for hiding this comment

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

The viewer should always be defined at this point. This is dangerous because it means that infoDialog would be false if the viewer is not defined, without any error message. If you want to satisfy the compiler, please use devAssert().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

devAssert() is making sense now. Thanks Gabriel!

? new InfoDialog(this.win, this.element)
: null;
if (infoDialog) {
Expand Down