🚫 [amp-story-player] Temporarily disable messaging#26604
🚫 [amp-story-player] Temporarily disable messaging#26604Enriqe merged 4 commits intoampproject:masterfrom
Conversation
|
This pull request introduces 1 alert when merging 6556e78 into 05046f6 - view on LGTM.com new alerts:
|
gmajoulet
left a comment
There was a problem hiding this comment.
LGTM, but this could have been three different PRs :)
|
This pull request introduces 1 alert when merging 7814258 into e0500b7 - view on LGTM.com new alerts:
|
newmuis
left a comment
There was a problem hiding this comment.
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
src/amp-story-player.js
Outdated
| removeFragment, | ||
| } from './url'; | ||
| // Source for this constant is css/amp-story-player-iframe.css | ||
| import {cssText} from '../build/amp-story-player-iframe.css'; |
There was a problem hiding this comment.
Should we still just import this as the name CSS? That's what we use in e.g. extension files
There was a problem hiding this comment.
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 🤷♀️
|
I split the css part into another PR #26613 👍 |
|
This pull request introduces 1 alert when merging 7046d12 into 9bc8756 - view on LGTM.com new alerts:
|
Partial for #24539 #26308