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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blocks: Add support for Collections #14454

Merged
merged 11 commits into from Apr 6, 2020
Merged

Blocks: Add support for Collections #14454

merged 11 commits into from Apr 6, 2020

Conversation

kraftbj
Copy link
Contributor

@kraftbj kraftbj commented Jan 24, 2020

Supports #14598
Replaces and closes #14721

In Gutenberg 7.3, the block editor now supports "Collections". In the block picker, a Collection looks like a category, except blocks can also have a category.

The use case here is when do people think "I need a business hours block. I bet that's in the Jetpack category!"... probably not. They probably think of the function -- an embed, a widget, etc.

The problem is a block can only be in one category so devs have generally grouped them by the block provider.

Collections allow the block provider (Jetpack in our case) present all of our blocks together—like we have done with a custom category—while also listing it in a context-appropriate category.

For those running Gutenberg 7.3 or WordPress 5.4:
2020-01-23 at 9 14 PM

For those not running Gutenberg 7.3 or on WordPress 5.3, it retains the original behavior:
2020-01-23 at 9 15 PM

Changes proposed in this Pull Request:

  • Adds a 'Jetpack' block collection.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • n/a

Testing instructions:

  • On a connected Jetpack site WITHOUT Gutenberg, add a new post. Verify all the blocks are in a Jetpack category when viewing the block inserter.
  • On a connected Jetpack site WITH Gutenberg, add a new post. With this patch, the Business Hours block will be in a Jetpack collection (looks just like a category) AND the widgets category.

AS OF NOW, when running G 7.3, the rest of Jetpack's blocks will fail since we're not adding them to a Jetpack category. We'll need to update the other blocks before landing it, but opening the PR now to get opinions on the implementation.

I opted to need to update blocks since if we allow those blocks with other categories to be added to a collection and those with the 'legacy' Jetpack category, the inserter will have two "Jetpack" entries -- the category and the collection -- so that's not really an option.

Proposed changelog entry for your changes:

  • Block Editor: Blocks are now listed both as part of a Jetpack Collection and in their proper category. (Requires Gutenberg 7.3)

@kraftbj kraftbj added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it General [Status] In Progress labels Jan 24, 2020
@kraftbj kraftbj requested a review from a team as a code owner January 24, 2020 03:19
@kraftbj kraftbj self-assigned this Jan 24, 2020
@jetpackbot
Copy link

jetpackbot commented Jan 24, 2020

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 3cdc453

@jeherve jeherve added the [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack label Jan 24, 2020
@scruffian
Copy link
Member

scruffian commented Jan 24, 2020

This might make sense to be done in tandem with this PR: WordPress/gutenberg#19279

Since the categories are set to change

@simison
Copy link
Member

simison commented Feb 25, 2020

When merged, we should confirm that re-organizing categories on wpcom is still functional: Automattic/wp-calypso#37057

@@ -82,7 +83,7 @@ export const settings = {
title: __( 'Business Hours', 'jetpack' ),
description: __( 'Display opening hours for your business.', 'jetpack' ),
icon,
category: 'jetpack',
category: typeof registerBlockCollection === 'function' ? 'widgets' : 'jetpack',
Copy link
Member

@simison simison Feb 25, 2020

Choose a reason for hiding this comment

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

I would centralize @wordpress/blocks import and typeof check into a helper function at extensions/shared/block-category.js and do supportsCollections() or similar. Not a biggie, just seems neater.

See example in coblocks: https://github.com/godaddy-wordpress/coblocks/pull/1367/files#diff-9996de4f1fe7baae35dfc23d3e0e3f3fR33

@simison
Copy link
Member

simison commented Mar 28, 2020

Would be nice to get this in before code freeze on Monday evening in time for WP 5.4 release which supports this feature. @lessbloat

@mtias
Copy link
Member

mtias commented Mar 28, 2020

Perhaps:

  • Calendly → Widgets
  • Contact Info → Formatting
  • Eventbrite Checkout → Widgets
  • Form → Widgets
  • GIF → Formatting
  • Google Calendar → Embeds
  • Map → Common
  • Markdown → Formatting
  • Open Table → Widgets
  • Pinterest → Embeds
  • Repeat Visitor → Widget
  • Revue → Widget
  • Slideshow → Common
  • Star Rating → Formatting
  • Tiled Gallery → Common

Register new category: Earn or Premium.

  • Ads
  • Simple Payments
  • Recurring Payments

@jeherve jeherve added this to the 8.4 milestone Mar 30, 2020
@jeherve jeherve mentioned this pull request Mar 30, 2020
14 tasks
@kraftbj
Copy link
Contributor Author

kraftbj commented Mar 30, 2020

Rebased and not finished yet, but need to log off.

What categories for these blocks:

  • Amazon
  • Business Hours (Widgets in this PR right now)
  • Instagram
  • Mailchimp
  • Podcast Player
  • Related Posts
  • Subscriptions

Still need to add the new category for Ads, Simple, Recurring Payments.

@apeatling
Copy link
Member

@kraftbj Let me know if you need any help getting this one finished off.

@obenland
Copy link
Member

obenland commented Apr 2, 2020

#14598 has a spreadsheet with suggested categories based on the discussion in p58i-8yl-p2. #14721 is the corresponding PR. It does away with the 'jetpack' category regardless of whether collections are available or not, as suggested in that post. Should we do the same here?

@kraftbj
Copy link
Contributor Author

kraftbj commented Apr 3, 2020

It does away with the 'jetpack' category regardless of whether collections are available or not, as suggested in that post. Should we do the same here?

I'm hesitant to do that since documentation notes you can find the JP blocks under "Jetpack" and existing users who aren't updated would lose that flow. When 5.4 is Jetpack's minimum, we can remove the fallback.

Thanks for the link to the spreadsheet.

@kraftbj
Copy link
Contributor Author

kraftbj commented Apr 3, 2020

Updated the PR with the two new categories and updating all blocks with the spreadsheet.

OpenTable struck me as odd being "Earn" instead of "Marketing" or "Embed", but included it as is.

Pinterest also was the only "social" one and there isn't that category yet. I made it "embed" instead for now.

Podcast Player wasn't in the spreadsheet. Marked it as an embed.

Rating Star wasn't in the spreadsheet. Deferred to the suggestion in the comment above.

Related Posts is listed as the only one in a non-existent "blog" category. Marked it as Embed.

@kraftbj kraftbj added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Apr 3, 2020
@Automattic Automattic deleted a comment from obenland Apr 3, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello kraftbj! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D41358-code before merging this PR. Thank you!

@kraftbj
Copy link
Contributor Author

kraftbj commented Apr 3, 2020

Noting that the wp.com patch needs a little massaging for the maps block. Nothing major, but I'll wait until this has landed to work on it so it isn't overridden with future work in this PR.

@matticbot
Copy link
Contributor

kraftbj, Your synced wpcom patch D41358-code has been updated.

@kraftbj kraftbj added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Apr 3, 2020
@simison
Copy link
Member

simison commented Apr 3, 2020

@jeherve there is separate script on wpcom that reorganizes categories in case we want to do that only there.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Let's add the textdomains here.

extensions/shared/block-category.js Outdated Show resolved Hide resolved
extensions/shared/block-category.js Outdated Show resolved Hide resolved
@matticbot
Copy link
Contributor

kraftbj, Your synced wpcom patch D41358-code has been updated.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This looks good to me now. I'd suggest merging this as is for the 8.4 release, and work on improving these collections in a future PR:

  • Handle wpcom differently
  • Have Jetpack blocks come after core blocks in core categories.

@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 Crew. Label will be renamed soon. labels Apr 6, 2020
@jeherve
Copy link
Member

jeherve commented Apr 6, 2020

Handle wpcom differently

To close the loop on this: #15310

Have Jetpack blocks come after core blocks in core categories.

I have been thinking about this some more, and I'm honestly not quite sure what we should aim for. Should Jetpack blocks be available before Core blocks in categories like Embeds?

image

My first instinct was to say no, our blocks should come after Core's. Core's blocks would most likely be used more often; they were added to Core more often. On the other hand I can see why having our own blocks first should give us some extra visibility.

Furthermore, Core currently adds custom blocks before core blocks in core categories, which seems to indicate that they're okay wit this?

@davemart-in
Copy link
Contributor

My first instinct was to say no, our blocks should come after Core's. Core's blocks would most likely be used more often; they were added to Core more often. On the other hand I can see why having our own blocks first should give us some extra visibility.

Could they just be listed alphabetically?

@jeherve
Copy link
Member

jeherve commented Apr 6, 2020

Could they just be listed alphabetically?

I suppose it could be another option. Would we then change all block ordering to alphabetical as soon as Jetpack is active, or only in categories that include Jetpack blocks?

@kraftbj kraftbj merged commit bc29cd5 into master Apr 6, 2020
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 6, 2020
kraftbj added a commit that referenced this pull request Apr 6, 2020
* Add Jetpack blocks collection

* Add Earn and Grow categories

Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
@kraftbj
Copy link
Contributor Author

kraftbj commented Apr 6, 2020

Cherry-picked to branch-8.4 via 9c91c10 .

@kraftbj
Copy link
Contributor Author

kraftbj commented Apr 6, 2020

r205423-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack General Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants