Skip to content

♻️ [Amp story shopping] Attachment as element child#36989

Merged
processprocess merged 25 commits intoampproject:mainfrom
processprocess:update-drawer
Nov 24, 2021
Merged

♻️ [Amp story shopping] Attachment as element child#36989
processprocess merged 25 commits intoampproject:mainfrom
processprocess:update-drawer

Conversation

@processprocess
Copy link
Copy Markdown
Contributor

@processprocess processprocess commented Nov 18, 2021 •
edited
Loading

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).

@processprocess processprocess self-assigned this Nov 18, 2021
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Nov 18, 2021 •
edited
Loading

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

@coreymasanto
Copy link
Copy Markdown
Contributor

coreymasanto commented Nov 18, 2021

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

@processprocess
Copy link
Copy Markdown
Contributor Author

processprocess commented Nov 18, 2021 •
edited
Loading

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.

gmajoulet
gmajoulet reviewed Nov 18, 2021
View reviewed changes
coreymasanto
coreymasanto requested changes Nov 22, 2021
View reviewed changes
coreymasanto
coreymasanto requested changes Nov 23, 2021
View reviewed changes
coreymasanto
coreymasanto approved these changes Nov 23, 2021
View reviewed changes
@processprocess processprocess merged commit bc73ec3 into ampproject:main Nov 24, 2021
@processprocess processprocess deleted the update-drawer branch November 24, 2021 18:38
@jshamble jshamble mentioned this pull request Nov 29, 2021
Closed
@mszylkowski mszylkowski mentioned this pull request Dec 6, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

@coreymasanto coreymasanto coreymasanto approved these changes

@alanorozco alanorozco Awaiting requested review from alanorozco

@jshamble jshamble Awaiting requested review from jshamble

Assignees

@processprocess processprocess

Labels

P1: High Priority PR use: In Beta / Experimental PR use: In LTS PR use: In Stable WG: stories

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@processprocess @coreymasanto @gmajoulet @ampprojectbot