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 new methods to new package #17126

Merged
merged 11 commits into from
Sep 17, 2020
Merged

Blocks: add new methods to new package #17126

merged 11 commits into from
Sep 17, 2020

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Sep 10, 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.

This just adds the method, we'll then start using them in blocks in #17101.

Jetpack product discussion

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

  • N/A

Testing instructions:

  • Not much to test yet, those methods aren't in use just yet.
  • Do the tests pass?

Proposed changelog entry for your changes:

  • N/A

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Pri] Normal AMP [Package] Blocks labels Sep 10, 2020
@jeherve jeherve added this to the 9.0 milestone Sep 10, 2020
@jeherve jeherve self-assigned this Sep 10, 2020
@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 10, 2020
@jeherve jeherve mentioned this pull request Sep 10, 2020
Merged
@jeherve jeherve added [Status] In Progress and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 10, 2020
@jetpackbot
Copy link

jetpackbot commented Sep 10, 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-17126

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 d56a0a9

@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 10, 2020
anomiex
anomiex reviewed Sep 14, 2020
View reviewed changes
packages/blocks/src/class-blocks.php Outdated Show resolved Hide resolved
packages/blocks/src/class-blocks.php Outdated Show resolved Hide resolved
packages/blocks/src/class-blocks.php Show resolved Hide resolved
packages/blocks/src/class-blocks.php Outdated Show resolved Hide resolved
packages/blocks/tests/php/test-blocks.php Outdated Show resolved Hide resolved
packages/blocks/tests/php/test-blocks.php Show resolved Hide resolved
packages/blocks/tests/php/test-blocks.php Outdated Show resolved Hide resolved
This was referenced Sep 15, 2020
Merged
Closed
anomiex
anomiex previously approved these changes Sep 15, 2020
View reviewed changes
Copy link
Contributor

@anomiex anomiex left a comment

Choose a reason for hiding this comment

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

Seems fine to me. I'm guessing the failing Travis test is the test somehow being flaky/broken since nothing seems to actually call the code being added here yet.

packages/blocks/tests/php/test-blocks.php Outdated Show resolved Hide resolved
@jeherve
Copy link
Member Author

jeherve commented Sep 15, 2020

I'm guessing the failing Travis test is the test somehow being flaky/broken since nothing seems to actually call the code being added here yet.

Yes, that e2e test can sometimes be a bit fragile.

anomiex
anomiex previously approved these changes Sep 15, 2020
View reviewed changes
@jeherve jeherve 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 16, 2020
jeherve pushed a commit that referenced this pull request Sep 16, 2020
@anomiex @jeherve
Move away from array_push
a4e3f9b 
See #17126 (comment)
@jeherve
Copy link
Member Author

jeherve commented Sep 16, 2020 •
edited
Loading

I had to rebase again; there were some Travis issues with the last build apparently. @anomiex Do you think you could review it one last time? Thanks!

@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 16, 2020
anomiex
anomiex previously approved these changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@anomiex anomiex left a comment

Choose a reason for hiding this comment

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

I see no changes other than the rebase, so re-approving. Travis still seems to be unhappy with that E2E test though.

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

jeherve commented Sep 16, 2020

On hold for now, pending changes to our e2e tests to take our new Plans into account (the new Plans screen is causing the e2e test to fail).

jeherve and others added 11 commits September 17, 2020 18:30
@jeherve
Move block_classes method to new Blocks package.
933a090
@jeherve
Add new method to check for AMP view from the package.
32f25d8
@jeherve
Add tests for new methods
7c1142b
@anomiex @jeherve
Move away from array_push
785c7a6 
See #17126 (comment)
@jeherve @anomiex
Fix typo
0e8b450 
Co-authored-by: Brad Jorsch <anomiex@users.noreply.github.com>
@jeherve
Update test with proper bar and baz attributes
826249c 
See #17126 (comment)
@jeherve
Add more assertions to cover all attributes
05858ec 
See #17126 (comment)
@jeherve
Add test for invalid alignment value.
d9805e9 
See #17126 (comment)
@jeherve
Ensure we always remove AMP filter, even when test fails.
e1d868a 
See #17126 (comment)
@anomiex @jeherve
Remove unneeded default
83fb164 
See #17126 (comment)
@anomiex @jeherve
Switch to alternative approach to handle potential test failures
d56a0a9 
See https://github.com/Automattic/jetpack/pull/17126/files#r488758175
@jeherve jeherve force-pushed the update/blocks-package-new-methods branch from f09531a to d56a0a9 Compare September 17, 2020 16:31
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Blocked / Hold labels Sep 17, 2020
@jeherve jeherve merged commit 508e933 into master Sep 17, 2020
@jeherve jeherve deleted the update/blocks-package-new-methods branch September 17, 2020 17:37
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brbrr brbrr brbrr approved these changes

@anomiex anomiex anomiex left review comments

Assignees

@jeherve jeherve

Labels
AMP [Package] Blocks [Pri] Normal [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [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 @jetpackbot @anomiex @brbrr @matticbot