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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃殌 [Story share] Move share menu to separate bundle #37179

Merged
merged 13 commits into from Dec 13, 2021

Conversation

mszylkowski
Copy link
Contributor

@mszylkowski mszylkowski commented Dec 10, 2021

Contributes to #37198
Closes #37203

Moving the share menu of amp-story to a new extension.

This PR will directly import the share menu on amp-story.js and not lazy load it, since we'd need to move more functionality between amp-story-share and amp-story-share-menu before that's possible (and the PR would get too large). Check the I2I for the steps.

This PR will improve the loading performance by not importing amp-social-share if the sharing is native, only if it's fallback. The native system sharing will be taken care of by the amp-story completely in a followup PR, but it's removed from the share menu in this PR to prevent moving unnecessary code.

Tested on Android (for system share) and Mac (for fallback share)

@mszylkowski mszylkowski self-assigned this Dec 13, 2021
@mszylkowski mszylkowski changed the title 馃殌 [Story share] Started to move share menu to separate bundle [WIP] 馃殌 [Story share] Move share menu to separate bundle Dec 13, 2021
@mszylkowski mszylkowski added this to In progress in wg-stories Sprint via automation Dec 13, 2021
@gmajoulet gmajoulet marked this pull request as ready for review December 13, 2021 21:42
@amp-owners-bot
Copy link

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-share.js
extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/test/test-amp-story-share-menu.js

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.

Consider this one approved, but I'll wait for formal approval so we submit all PRs at once :))

@mszylkowski mszylkowski merged commit 584f5a6 into ampproject:main Dec 13, 2021
wg-stories Sprint automation moved this from In progress to Done Dec 13, 2021
@mszylkowski mszylkowski deleted the removeShareStart branch December 13, 2021 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Create new extension and move share menu to it.
3 participants