🚀♻️ Extract amp-story-interactive host styles into extension#37051
🚀♻️ Extract amp-story-interactive host styles into extension#37051alanorozco wants to merge 5 commits intoampproject:mainfrom
amp-story-interactive host styles into extension#37051Conversation
|
Hey @gmajoulet! These files were changed: Hey @mszylkowski! These files were changed: Hey @newmuis! These files were changed: |
| * @param {string} cssText | ||
| */ | ||
| function installHostCssOnce(id, cssRoot, sibling, cssText) { | ||
| if (cssRoot.querySelector(`#${escapeCssSelectorIdent(id)}`)) { |
There was a problem hiding this comment.
I think it would work well if we kept the system style[amp-extension="amp-story-interactive"] to install the styles, why are we doing this instead?
There was a problem hiding this comment.
There are two options to use the existing style[amp-extension="amp-story-interactive"] system:
-
By bundling
installExtensionsForDoc(), which has a~0.16 kbimpact onamp-story-interactive-1.0.js. -
By placing the host CSS in every inherited component, which has a variable impact on runtime performance by overriding styles once for every tag name defined in the extension (10x).
I think the size impact on amp-story-interactive-1.0.js is a more reasonable tradeoff, so I chose to write a function to install once. However, the hand-rolled function is barely smaller, so I'm now using installExtensionsForDoc() instead.
| /** @protected {?../../amp-story/1.0/variable-service.AmpStoryVariableService} */ | ||
| this.variableService_ = null; | ||
|
|
||
| installHostCssOnce( |
There was a problem hiding this comment.
I thought you were going to call this in amp-story-interactive.js before running the registerElements so it's guaranteed to be installed once, instead of trying to install it once per component.
There was a problem hiding this comment.
I was following the behavior of instalStylesOrDoc, which verifies whether the style has already been added.
Subjectively, it makes more sense to me to place the call in the abstract implementation instead of AMP.extension(). It's uniform with shadow styles, and it mostly corresponds to the classnames set in the same file.
gmajoulet
left a comment
There was a problem hiding this comment.
This is cool, let's ship it please!!
|
@alanorozco whenever you get the chance, this PR is approved and (after conflict resolution) ready to merge |
|
Friendly ping on this, would love to ship it : )) |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
The provided custom props are not required as part of the larger
amp-story-1.0.jsbundle. We move them into theamp-story-interactive-1.0.jsbundle instead.This shaves off
~0.55 kbcompressed from the larger bundle.