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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Blocks: add AMP and CSS Classes methods to the new package #17101

Merged
merged 1 commit into from
Sep 22, 2020
Merged

Blocks: add AMP and CSS Classes methods to the new package #17101

merged 1 commit into from
Sep 22, 2020

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Sep 8, 2020 •
edited
Loading

Changes proposed in this Pull Request:

  • This PR starts adding methods to the new Blocks package we'll use for all Jetpack blocks from now on.

Jetpack product discussion

Does this pull request change what data or activity we track or use?

  • N/A

Testing instructions:

  • Install the AMP Plugin on your site.
  • Connect Jetpack to WordPress.com, and try to add all the blocks added as labels to this PR.
  • View them in AMP views as well.
  • Ensure all blocks are displayed properly.
  • Ensure that blocks that include a view.js file (such as the Map block) still load it on non-AMP views.

Proposed changelog entry for your changes:

  • N/A

@jeherve jeherve added this to the 9.0 milestone Sep 8, 2020
@jeherve jeherve self-assigned this Sep 8, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D49236-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Sep 8, 2020
@jetpackbot
Copy link

jetpackbot commented Sep 8, 2020 •
edited
Loading

Scheduled Jetpack release: October 6, 2020.
Scheduled code freeze: September 29, 2020

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17101

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Generated by 🚫 dangerJS against 7454da8

@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Sep 8, 2020
@jeherve jeherve mentioned this pull request Sep 8, 2020
Closed
@jeherve jeherve force-pushed the update/blocks-package branch from 1c258c8 to ea4ecbc Compare September 10, 2020 15:07
@jeherve jeherve changed the base branch from master to update/blocks-package-new-methods September 10, 2020 15:07
@jeherve jeherve force-pushed the update/blocks-package branch 2 times, most recently from f0d68a9 to b540d71 Compare September 15, 2020 16:14
@jeherve jeherve force-pushed the update/blocks-package-new-methods branch from 17e8d4a to f09531a Compare September 16, 2020 09:32
@jeherve jeherve force-pushed the update/blocks-package branch 2 times, most recently from 792da53 to 0a4c79c Compare September 16, 2020 09:48
@jeherve jeherve force-pushed the update/blocks-package-new-methods branch from f09531a to d56a0a9 Compare September 17, 2020 16:31
Base automatically changed from update/blocks-package-new-methods to master September 17, 2020 17:37
@jeherve jeherve force-pushed the update/blocks-package branch from 0a4c79c to 837362d Compare September 17, 2020 17:48
@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Blocked / Hold labels Sep 17, 2020
@jeherve jeherve force-pushed the update/blocks-package branch from 837362d to 7454da8 Compare September 22, 2020 09:12
fgiannar
fgiannar approved these changes Sep 22, 2020
View reviewed changes
Copy link
Contributor

@fgiannar fgiannar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested all blocks added as labels in this PR before and after applying the changes introduced in this PR and can confirm the blocks are displayed properly with the following 2 exceptions:

  • Google calendar block: This block is removed from AMP view due to missing width
  • Opentable block: This block is removed from AMP view due to invalid script

However, the above are not related with the present PR so it can be approved. Just mentioning them here as a reference, please @jeherve let me know if I should create an issue for those.

@fgiannar fgiannar added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 22, 2020
@jeherve
Copy link
Member Author

jeherve commented Sep 22, 2020

Opentable block: This block is removed from AMP view due to invalid script

This should be addressed in #17085

Google calendar block: This block is removed from AMP view due to missing width

Nothing in progress for this one, but it's tracked in #14395.

@jeherve jeherve merged commit 92de5f8 into master Sep 22, 2020
@jeherve jeherve deleted the update/blocks-package branch September 22, 2020 11:07
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 22, 2020
@jeherve jeherve mentioned this pull request Sep 22, 2020
Merged
@anomiex anomiex mentioned this pull request Sep 23, 2020
Merged
@kraftbj
Copy link
Contributor

kraftbj commented Sep 23, 2020

@jeherve How did you update the composer.lock so the branch was changed to dev-master?

@jeherve
Copy link
Member Author

jeherve commented Sep 24, 2020

I don't know, I don't really remember doing anything specific. I believe I just composer install?

@kraftbj
Copy link
Contributor

kraftbj commented Sep 24, 2020

:y: It looked almost like it was ran standing on master? ¯_(ツ)_/¯

@jeherve
Copy link
Member Author

jeherve commented Oct 6, 2020

r214736-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgiannar fgiannar fgiannar approved these changes

Assignees

@jeherve jeherve

Labels
AMP [Block] Button [Block] Calendly [Block] Eventbrite [Block] GIF [Block] Google Calendar [Block] Image Compare [Block] Latest Instagram Posts [Block] Mailchimp [Block] Map [Block] OpenTable [Block] Pinterest [Block] Podcast Player [Block] Repeat Visitor [Block] Revue [Block] Slideshow [Block] Story [Package] Blocks [Pri] Normal [Status] Needs Package Release This PR made changes to a package. Let's update that package now. Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
9.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jeherve @matticbot @jetpackbot @kraftbj @fgiannar