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: update all blocks to use namespaces and constants for consistency #15088

Merged
merged 7 commits into from
Mar 23, 2020
Merged

Blocks: update all blocks to use namespaces and constants for consistency #15088

merged 7 commits into from
Mar 23, 2020

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Mar 23, 2020 •
edited
Loading

Fixes #14843

Changes proposed in this Pull Request:

This PR does not introduce any new functionality or changes anything in the way the blocks work today. It does, however, change the way blocks are registered to bring all blocks to the same level and be consistent across all blocks. It should help new Jetpack contributors who work on blocks, by giving them one approach they can follow to register a new block.

  • I've updated the template used when using WP Cli to create a new block. This is work that was already started in Block scaffolding: switch to namespaces for block registration #14728.
  • I've updated all blocks to be under the Automattic\Jetpack namespace (vs. just Jetpack before, or no namespace at all). Adding the vendor name to the namespace is how we work with all other namespaces in Jetpack. Part of me wonders if we shouldn't introduce one more level there, so all blocks live under an Automattic\Jetpack\Blocks or Automattic\Jetpack\Extensions namespace. It may make things easier if one day we move those blocks into a package.

Testing instructions:

  • You should be able to use all those blocks with no issues or notices in your logs:
    • Amazon (Beta)
    • Business Hours
    • Calendly
    • Eventbrite
    • Gif
    • Google Calendar
    • Instagram Gallery (Beta)
    • Mailchimp
    • Map
    • Markdown
    • Opentable
    • Pinterest
    • Podcast Player (Beta)
    • Rating Star
    • Repeat Visitor
    • Revue
    • Slideshow
    • Tiled Gallery
    • WordAds

Proposed changelog entry for your changes:

  • N/A

@jeherve jeherve added [Status] Needs Review This PR is ready for review. [Type] Janitorial [Pri] Normal [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Mar 23, 2020
@jeherve jeherve added this to the 8.4 milestone Mar 23, 2020
@jeherve jeherve requested a review from a team March 23, 2020 12:04
@jeherve jeherve self-assigned this Mar 23, 2020
jeherve
jeherve commented Mar 23, 2020
View reviewed changes
extensions/blocks/business-hours/business-hours.php
Comment on lines -122 to +138
date( $time_format, $opening ),
date( $time_format, $closing )
gmdate( $time_format, $opening ),
gmdate( $time_format, $closing )
Copy link
Member Author

Choose a reason for hiding this comment

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

This was necessary as it was triggering a PHPCS error that stopped me from committing.

@jetpackbot
Copy link
Collaborator

jetpackbot commented Mar 23, 2020 •
edited
Loading

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.

Scheduled Jetpack release: April 7, 2020.
Scheduled code freeze: March 31, 2020

Generated by 🚫 dangerJS against 62b057f

jeherve
jeherve commented Mar 23, 2020
View reviewed changes
extensions/blocks/tiled-gallery/tiled-gallery.php
* Tiled Gallery block. Depends on the Photon module.
* Tiled Gallery block.
* Relies on Photon, but can be used even when the module is not active.
Copy link
Member Author

Choose a reason for hiding this comment

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

I took the opportunity to update this since it wasn't valid anymore since #13471.

@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, review, and approve D40681-code before merging this PR. Thank you!

leogermani
leogermani previously approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Looked at the code and tested the blocks. Looks fine to me!

@kraftbj kraftbj self-requested a review March 23, 2020 15:58
kraftbj
kraftbj previously approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Able to add and save a number of blocks. No items in the PHP log.

@kraftbj kraftbj added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 23, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Mar 23, 2020

Marked as needing author reply so we can restart Travis and get wpcom Fusion happy, but it can be merged once those things are green.

@matticbot
Copy link
Contributor

jeherve, Your synced wpcom patch D40681-code has been updated.

@jeherve jeherve force-pushed the update/blocks-use-namespace branch from 6b144fe to d87b3a6 Compare March 23, 2020 17:47
@matticbot
Copy link
Contributor

jeherve, Your synced wpcom patch D40681-code has been updated.

@jeherve jeherve removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 23, 2020
@jeherve jeherve added the [Status] Needs Review This PR is ready for review. label Mar 23, 2020
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 23, 2020
@jeherve jeherve merged commit 70dab92 into master Mar 23, 2020
@jeherve jeherve deleted the update/blocks-use-namespace branch March 23, 2020 19:04
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 23, 2020
@jeherve
Copy link
Member Author

jeherve commented Mar 23, 2020

r204659-wpcom

@jeherve jeherve mentioned this pull request Mar 23, 2020
Merged
7 tasks
@kraftbj kraftbj mentioned this pull request Mar 23, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj approved these changes

@leogermani leogermani leogermani left review comments

Assignees

@jeherve jeherve

Labels
[Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Pri] Normal Touches WP.com Files [Type] Janitorial
Projects
None yet
Milestone
8.4
Development

Successfully merging this pull request may close these issues.

Block registration: update file structure to match latest template
5 participants
@jeherve @jetpackbot @matticbot @kraftbj @leogermani