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

🚫 [amp-story-player] Temporarily disable messaging #26604

Merged
merged 4 commits into from
Feb 4, 2020
Merged

🚫 [amp-story-player] Temporarily disable messaging #26604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c58205c
temporarily disable messaging, add separate iframe css file
Enriqe Feb 3, 2020
6556e78
update owners
Enriqe Feb 3, 2020
7814258
skip tests, link todos to issue tracker
Enriqe Feb 3, 2020
7046d12
remove css related changes
Enriqe Feb 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions examples/amp-story/player.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@
<h1>This is a NON-AMP page that embeds a story below:</h1>
<div>Lorem ipsum dolor sit amet, consectetur adipiscing elit. Cras viverra neque ex, sit amet varius sem maximus sed. Suspendisse potenti. Donec erat purus, sagittis sit amet tincidunt ut, maximus sit amet massa. Maecenas venenatis fringilla dui vitae vestibulum. Fusce imperdiet euismod lobortis. Nullam sagittis nunc at tristique mattis. In sodales consectetur mollis. Maecenas sollicitudin, ex vel tempor rutrum, turpis eros interdum enim, sit amet volutpat tortor dolor at ipsum. Nam posuere velit vel urna vulputate interdum. Aenean eu vulputate lorem. Praesent nec nunc sodales, egestas orci sed, hendrerit mauris. Ut blandit turpis non erat sagittis, quis fermentum odio feugiat.</div>
<amp-story-player style="width: 360px; height: 600px;">
<a
href="https://www-washingtonpost-com.cdn.ampproject.org/v/s/www.washingtonpost.com/graphics/2019/lifestyle/travel/amp-stories/a-locals-guide-to-what-to-eat-and-do-in-new-york-city/"
<a href="https://www.washingtonpost.com/graphics/2019/lifestyle/travel/amp-stories/a-locals-guide-to-what-to-eat-and-do-in-new-york-city/"
style="--story-player-poster: url('https://www-washingtonpost-com.cdn.ampproject.org/i/s/www.washingtonpost.com/graphics/2019/lifestyle/travel/amp-stories/a-locals-guide-to-what-to-eat-and-do-in-new-york-city/img/promo3x4.jpg');"
class="story">
<span class="title">A local’s guide to what to eat and do in New York City</span>
Expand Down
14 changes: 9 additions & 5 deletions src/amp-story-player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class AmpStoryPlayer {

this.initializeShadowRoot_();

// TODO(Enriqe): Build all child iframes.
// TODO(#26308): Build all child iframes.
this.buildIframe_(this.stories_[0]);
}

Expand Down Expand Up @@ -134,6 +134,9 @@ export class AmpStoryPlayer {
this.initializeLoadingListeners_(iframeEl);
this.rootEl_.appendChild(iframeEl);

// TODO(#26308): enable messaging when multiple documents are supported.
return;

this.initializeHandshake_(story, iframeEl).then(
messaging => {
const iframeIdx = findIndex(
Expand All @@ -143,7 +146,7 @@ export class AmpStoryPlayer {

this.messagingFor_[iframeIdx] = messaging;

// TODO(Enriqe): Appropiately set visibility to stories.
// TODO(#26308): Appropiately set visibility to stories.
this.displayStory_(iframeIdx);
},
err => {
Expand Down Expand Up @@ -196,7 +199,7 @@ export class AmpStoryPlayer {
return;
}

// TODO(Enriqe): Layout all child iframes.
// TODO(#26308): Layout all child iframes.
this.layoutIframe_(this.stories_[0], this.iframes_[0]);

this.isLaidOut_ = true;
Expand All @@ -208,9 +211,10 @@ export class AmpStoryPlayer {
* @private
*/
layoutIframe_(story, iframe) {
const {href} = this.getEncodedLocation_(story.href);
// TODO(#26308): enable messaging when multiple documents are supported.
// const {href} = this.getEncodedLocation_(story.href);

iframe.setAttribute('src', href);
iframe.setAttribute('src', story.href);
}

/**
Expand Down
6 changes: 4 additions & 2 deletions test/unit/test-amp-story-player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ describes.realWin('AmpStoryPlayer', {amp: false}, env => {
expect(playerEl.shadowRoot.querySelector('iframe')).to.exist;
});

it('should correctly append params at the end of the story url', () => {
// TODO(#26308): unskip when messaging is enabled.
it.skip('should correctly append params at the end of the story url', () => {
manager.loadPlayers();
const storyIframe = playerEl.shadowRoot.querySelector('iframe');

Expand All @@ -53,7 +54,8 @@ describes.realWin('AmpStoryPlayer', {amp: false}, env => {
);
});

it('should correctly append params at the end of a story url with existing params', () => {
// TODO(#26308): unskip when messaging is enabled.
it.skip('should correctly append params at the end of a story url with existing params', () => {
url += '?testParam=true#myhash=hashValue';
playerEl.firstElementChild.setAttribute('href', url);

Expand Down