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 paid label next to block title when it requires a plan #14739

Merged
merged 7 commits into from
Mar 4, 2020

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Feb 19, 2020

Changes proposed in this Pull Request:

Related: #11875, #13490

  • This adds a (paid) label next to block titles whenever a block requires a plan.

screenshot 2020-02-19 at 22 38 15

A few notes:

  • I have chosen to have (beta) take priority whenever a block is both paid and in Beta. I am not sure that's the best approach.
  • (beta) is not translatable. I don't know if it should be.

@jeryj Do you think you could take a look and take over?

Thank you!

Testing instructions:

  • Start from a site using the free version of Jetpack, and connected to WordPress.com.
  • Add the following filter to enable paid blocks on your site: add_filter( 'jetpack_block_editor_enable_upgrade_nudge', '__return_true' );
  • Open the block picker.

Proposed changelog entry for your changes:

  • Blocks: better differentiate paid blocks from free ones.

@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 Crew. Label will be renamed soon. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Feb 19, 2020
@jeherve jeherve requested a review from a team as a code owner February 19, 2020 21:43
@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 D39107-code before merging this PR. Thank you!

@jetpackbot
Copy link

jetpackbot commented Feb 19, 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 25129a6

@jeryj
Copy link
Contributor

jeryj commented Feb 19, 2020

Thank you for getting this started and providing direction, @jeherve! I have a questions that is likely just related to my newness to Jetpack. The only paid plan block I'm seeing is Recurring Payments, and it's not showing the (paid) label.

This is for both my local docker ngrok'd site and the PR-generated site. They're both connected to my WP account via a free Jetpack plan. I've looked in both wp-admin and WordPress.com's editor for more beta and upgrade-required blocks. Am I looking in the wrong area, or is there some other set-up I need to do to see more beta and paid blocks like in your screenshot?

@jeherve
Copy link
Member Author

jeherve commented Feb 20, 2020

I've added a note to the PR description: to see paid blocks when you don't have a plan in Jetpack, you need to use this filter:
add_filter( 'jetpack_block_editor_enable_upgrade_nudge', '__return_true' );

Beta blocks, on the other hand, can be enabled by adding the following to your site's wp-config.php file:
define( 'JETPACK_BETA_BLOCKS', true );

@WunderBart
Copy link
Member

Thanks for opening this PR and for the detailed description, @jeherve! 🙌

I have chosen to have (beta) take priority whenever a block is both paid and in Beta. I am not sure that's the best approach.

How about joining those tags/badges/labels? We'd get something like:

image
(not sure about the order here ☝️)

(beta) is not translatable. I don't know if it should be.

I'm not sure here either, but when I drop it in Google Translate, e.g. the Russian version is "бета". I guess the question is if we want to make it translatable?


For the implementation of the above, how about creating blockTags object that would contain (translated?) strings, e.g.:

const blockTags = {
  paid: __( 'paid' ),
  beta: __( 'beta' ),
};

...and append (& join) them accordingly to the block's name when needed. What do you think?
cc @davemart-in

@davemart-in
Copy link
Contributor

Thanks for kicking this off @jeherve! Combining beta and paid when appropriate sounds fine.

@matticbot
Copy link
Contributor

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

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

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

…BlockTags.paid

The availableBlockTags object has the property paid, and not plan. The check to add the (paid) label was looking for the availableBlockTags.plan property, which was undefined. I updated this to look for availableBlockTags.paid.
@matticbot
Copy link
Contributor

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

@jeryj
Copy link
Contributor

jeryj commented Mar 3, 2020

I made the requested changes to the code. I think this is ready for another round of review 👍

@jeryj jeryj 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 Mar 3, 2020
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Nice job. Looks good from a code perspective. I haven't been able to test manually yet. Still working on env setup.

@@ -44,9 +57,23 @@ export default function registerJetpackBlock( name, settings, childBlocks = [] )
return false;
}

let blockTitle = settings.title;
const blockTags = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: it might seem obvious when you've been working on it for a while, but I wonder whether it could be helpful to add a comment here detailing what we're doing here. It was immediately obvious to me even though I had some context. For others it could be even less clear.

For example:

Some Blocks should be "tagged" to highlight various attributes such as being part of a paid plan or being in a beta programme. We add these to the Block's visible title in order that they show up in the picker.

Alternatively, perhaps it would be better to handle adding "tags" in a dedicated method. This might remove the need to document with a comment as the code would be self describing. Perhaps:

buildBlockTags and buildBlockTitle?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've refactored the implementation here: 25129a6

This adds buildBlockTags and buildBlockTitle functions and builds the title with buildBlockTitle( settings.title, buildBlockTags( name, requiredPlan ) )

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Mar 4, 2020
…solated build functions.

- Adds buildBlockTags( name, requiredPlan)
- Adds buildBlockTitle( blockTitle, blockTags )
- Builds the title in registerJetpackBlock with buildBlockTitle( settings.title, buildBlockTags( name, requiredPlan ) )
@matticbot
Copy link
Contributor

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

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Looks much better. Thanks for the refactor 👍

I'm still setting up my env for testing so please can you source another manual review?

I'm sorry about this.

*/
function buildBlockTitle( blockTitle, blockTags = [] ) {
if ( blockTags.length ) {
blockTitle = `${ blockTitle } (${ blockTags.join( ', ' ) })`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
blockTitle = `${ blockTitle } (${ blockTags.join( ', ' ) })`;
blockTitle = `${ blockTitle } (${ blockTags.join() })`;

If omitted, the array elements are separated with a comma

Copy link
Contributor

Choose a reason for hiding this comment

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

True, but there's no space between them, so it would end up being (paid,beta) instead of (paid, beta)

Copy link
Member

@WunderBart WunderBart left a comment

Choose a reason for hiding this comment

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

Tested locally, everything looks great! :shipit:

@jeryj
Copy link
Contributor

jeryj commented Mar 4, 2020

I refactored and I believe I addressed all the raised comments. Could I please get another review? 🙏

@jeryj jeryj 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 Mar 4, 2020
@jeherve jeherve added this to the 8.4 milestone Mar 4, 2020
@kraftbj kraftbj 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 Mar 4, 2020
@jeryj jeryj merged commit 0fd1874 into master Mar 4, 2020
@jeryj jeryj deleted the add/paid-mention-blocks branch March 4, 2020 17:31
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 4, 2020
@jeherve
Copy link
Member Author

jeherve commented Mar 6, 2020

r203949-wpcom

jeherve added a commit that referenced this pull request Mar 23, 2020
jeherve added a commit that referenced this pull request Mar 31, 2020
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
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 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.

10 participants