♻️ [Amp story shopping] Attachment as element child by processprocess · Pull Request #36989 · ampproject/amphtml (original) (raw)

Refactor to use an element child instead of class inheritance.
This reduces bundle size by removing the import of AmpStoryPageAttachent as well as potential technical debt from a third level of inheritance.
(draggable-drawer > page-attachment > shopping-attachment).

Hey @gmajoulet! These files were changed:

extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.css
extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-attachment.js
extensions/amp-story/1.0/amp-story-draggable-drawer.js

Hey @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-draggable-drawer.js

Out of curiosity, how does this refactor reduce the bundle size?

Also, is there a technical concern about using inheritance here? This PR takes the existing structure of
BaseElement > draggable-drawer > page-attachment > shopping-attachment
and updates it to
BaseElement > draggable-drawer > page-attachment
BaseElement > shopping-attachment.
The updated structure seems like it may be more difficult to reason about during implementation

It reduces the bundle size by removing the import of AmpStoryPageAttachent.

It can be implemented either way but the idea behind this approach is to favor composition over inheritance.
The only function we need to override is open. Other than that we will be switching out content within the attachment and editing the header.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})