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.

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

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

Merged
merged 4 commits into from
Feb 4, 2020

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Feb 3, 2020

  • Temporarily disables messaging until we add support for multiple documents.

Partial for #24539 #26308

@lgtm-com
Copy link

lgtm-com bot commented Feb 3, 2020

This pull request introduces 1 alert when merging 6556e78 into 05046f6 - view on LGTM.com

new alerts:

  • 1 for Unreachable statement

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

LGTM, but this could have been three different PRs :)

css/amp-story-player.css Outdated Show resolved Hide resolved
src/amp-story-player.js Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Feb 3, 2020

This pull request introduces 1 alert when merging 7814258 into e0500b7 - view on LGTM.com

new alerts:

  • 1 for Unreachable statement

Copy link
Contributor

@newmuis newmuis left a comment

Choose a reason for hiding this comment

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

I think I'm ok with this, though it really seems like it could be separated into two PRs: CSS for changes and removing viewer messaging

@@ -22,6 +22,8 @@ import {
parseUrlWithA,
removeFragment,
} from './url';
// Source for this constant is css/amp-story-player-iframe.css
import {cssText} from '../build/amp-story-player-iframe.css';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we still just import this as the name CSS? That's what we use in e.g. extension files

Copy link
Member

Choose a reason for hiding this comment

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

cssText is the exported name when defined in build-system/tasks/css.js. Extension CSS goes through a different part of the build pipeline that names it CSS 🤷‍♀️

@Enriqe Enriqe changed the title 🖍 [amp-story-player] Temporarily disable messaging, add separate iframe css file 🖍 [amp-story-player] Temporarily disable messaging Feb 4, 2020
@Enriqe
Copy link
Contributor Author

Enriqe commented Feb 4, 2020

I split the css part into another PR #26613 👍

@Enriqe Enriqe changed the title 🖍 [amp-story-player] Temporarily disable messaging 🚫 [amp-story-player] Temporarily disable messaging Feb 4, 2020
@lgtm-com
Copy link

lgtm-com bot commented Feb 4, 2020

This pull request introduces 1 alert when merging 7046d12 into 9bc8756 - view on LGTM.com

new alerts:

  • 1 for Unreachable statement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants