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
馃悰 Move ampDoc references into amp-story buildCallback #30986
馃悰 Move ampDoc references into amp-story buildCallback #30986
Conversation
Hey @gmajoulet, @newmuis, @Enriqe! These files were changed:
|
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.
LGTM pending the one comment
@@ -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_) |
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.
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()
.
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.
devAssert()
is making sense now. Thanks Gabriel!
* Move methods that need ampdoc to buildCallback. * Update types. * Remove const * Fix typo. * devAssert.
context / fixes #30798