feat: Enable Jetpack collection for native block inserter #25092

dcalhoun · 2022-07-13T21:01:48Z

Display a collection of Jetpack blocks within the native block inserter to increase brand awareness during the Jetpack app migration.

Changes proposed in this Pull Request:

The web editor displays a collection of Jetpack blocks in the block inserter for non-WordPress.com, Jetpack-connected sites. This change enables that same collection display for the native block inserter.
Rename the collection from "Jetpack" to "Jetpack powered" for the native block inserter.
Add a separate block-category.native.js file to configure native platform without adding additional code/complexity to the web bundle.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?

Jetpack product discussion

p9ugOq-33V-p2

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

No.

Testing instructions:

See wordpress-mobile/gutenberg-mobile#5024.

matticbot · 2022-07-13T21:01:55Z

Caution: This PR has changes that must be merged to WordPress.com
Hello dcalhoun! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D84214-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 · 2022-07-13T21:03:06Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
⚠️ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: August 2, 2022.
Scheduled code freeze: July 25, 2022.

The native editor now displays collections within the block inserter. Specifically, a collection of Jetpack blocks is displayed with "Jetpack powered" badge within the inserter.

Changes green background to white.

The collection title is now rendered to make dark mode support easier.

dcalhoun · 2022-07-15T01:06:31Z

projects/plugins/jetpack/extensions/shared/block-category.native.js

+registerBlockCollection( 'jetpack', {
+	title: 'Jetpack powered',
+	icon: <JetpackLogo />,
+} );


There are only two differences between this native file and the sibling web file:

The isWpCom conditional is removed (this check relies upon window and does not work for the native editor).

Changing the collection title from "Jetpack" to "Jetpack powered."

A separate native file feels a bit excessive for such small differences, but it is likely for the best so that (1) the web bundle size does not increase with native code and (2) a misleading isWpCom no-op is not run for the native editor.

@dcalhoun I'm wondering if this should only be enabled for the WordPress app, probably showing the Jetpack logo in the Jetpack app would be a bit redundant, wdyt?

+1 to this seeming a bit redundant for the Jetpack app, but not necessarily a blocker to merging if limiting it to only the WP app proves to be complex (especially as the eventual plan is to remove it).

That is a great thought. I agree displaying "Jetpack powered" within the Jetpack app may be redundant.

@fluiddot @SiobhyB what approach do you think might be appropriate for controlling this?

We would need to set/access a value indicating the WordPress/Jetpack app and/or site type prior to the point we invoke block collection registration.

I perceive the capabilities to be of similar nature, but they target individual features, and do not describe a higher-level app/site "type." Relatedly, the Jetpack plugin houses a set of utilities for determining site type, but they are incompatible with the native context.

We could possibly check each of the Jetpack block capabilities...

if (mediaFilesCollectionBlock || contactInfoBlock || tiledGalleryBlock) { // register Jetpack block collection }

...or add a new Jetpack site/app capability.

if (capabilities.jetpack) { // register Jetpack block collection }

Are there alternative approaches I am overlooking?

+1 to this seeming a bit redundant for the Jetpack app, but not necessarily a blocker to merging if limiting it to only the WP app proves to be complex (especially as the eventual plan is to remove it).

Good point, I also think that only enabling it for the WP app shouldn't be a blocker.

Are there alternative approaches I am overlooking?

I noticed that in the iOS native side we're using an object named AppConfiguration that contains different flags, including isJetpack to determine if the app is the Jetpack one. As an example, this flag is being used for determining when the Jetpack banner should be displayed (reference), so we could use that value for this too.

In relation to how we could pass this information to RN side, I was thinking to have a new capability or initial prop to identify the client. This way we could enable/disable the block collection depending on the client (i.e. the app), wdyt?

I noticed that in the iOS native side we're using an object named AppConfiguration that contains different flags, including isJetpack to determine if the app is the Jetpack one.

Agreed. This does seem the most appropriate flag to use. Thanks for sharing.

In relation to how we could pass this information to RN side, I was thinking to have a new capability or initial prop to identify the client. This way we could enable/disable the block collection depending on the client (i.e. the app), wdyt?

As I consider it further, setting the app type within capabilities feels a bit inappropriate. I imagine it makes more sense to have a isJetpack boolean within the editor props/settings.

I can begin work to limit when the "Jetpack powered" badge displays, but it looks like the consensus is that work should not block merging this work. 👍🏻

As I consider it further, setting the app type within capabilities feels a bit inappropriate. I imagine it makes more sense to have a isJetpack boolean within the editor props/settings.

True, although this would imply adding it to Gutenberg and hence, add a reference to Jetpack 😅. An alternative could be to generate a different RN bundle for each WordPress and Jetpack app. This way we could set global constants similar to how it's done on the native side with AppConfiguration for each app. In fact, this option might be necessary in the future for overriding the colors of the editor for example.

I can begin work to limit when the "Jetpack powered" badge displays, but it looks like the consensus is that work should not block merging this work. 👍🏻

Yep, let's wrap up this task first and then focus on how we can identify in the editor the app that is being run 👍 .

True, although this would imply adding it to Gutenberg and hence, add a reference to Jetpack 😅.

I wondered this myself. I hoped we might be able to strip out the value within gutenberg-mobile's editor setup code, but we may still run into issue where the new property may have to be defined within react-native-bridge protocols.

An alternative could be to generate a different RN bundle for each WordPress and Jetpack app. This way we could set global constants similar to how it's done on the native side with AppConfiguration for each app. In fact, this option might be necessary in the future for overriding the colors of the editor for example.

Another alternative might be to define a generic host app identifier, e.g. hostAppNamespace: 'jetpack'|'wordpress'|'awesome-future-app'.

Another alternative might be to define a generic host app identifier, e.g. hostAppNamespace: 'jetpack'|'wordpress'|'awesome-future-app'.

Yeah, this is aligned to what I commented about passing the editor's client, although I agree that hostAppNamespace sounds more accurate.

SiobhyB

Approving following the review at WordPress/gutenberg#42405 (review), with the understanding there will likely be a follow up task to limit this change to the WP app.

In addition, we'll need to get approval from the Jetpack team before this PR's merged (the [Status] Needs Review label can be used to get this final review).

…nserter

dcalhoun · 2022-07-15T15:35:13Z

Thank you, Siobhan! Marking this as ready for review by the Jetpack crew now.

Note for the crew: I don't have commit access for WP.com, so would be grateful if you could handle committing to WP.com for me too. 🙇🏻

projects/plugins/jetpack/extensions/shared/block-category.native.js

Co-authored-by: Jeremy Herve <[email protected]>

jeherve · 2022-07-15T15:49:59Z

r249105-wpcom

github-actions · 2022-07-15T15:56:48Z

Great news! One last step: head over to your WordPress.com diff, D84214-code, and deploy it.
Once you've done so, come back to this PR and add a comment with your SVN changeset ID (e.g. r12345-wpcom).

Thank you!

dcalhoun added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Jul 13, 2022

dcalhoun self-assigned this Jul 13, 2022

matticbot added Touches WP.com Files labels Jul 13, 2022

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jul 13, 2022

dcalhoun added the [Status] In Progress label Jul 13, 2022

dcalhoun mentioned this pull request Jul 14, 2022

feat: Inserter displays block collections wordpress-mobile/gutenberg-mobile#5024

Merged

dcalhoun added 3 commits July 14, 2022 18:07

[not verified] feat: Enable Jetpack collection for native block inserter

577ac7b

The native editor now displays collections within the block inserter. Specifically, a collection of Jetpack blocks is displayed with "Jetpack powered" badge within the inserter.

[not verified] changelog

e6d5b75

[not verified] chore: Update JetpackPoweredBadge asset

ac90801

Changes green background to white.

dcalhoun force-pushed the feat/enable-jetpack-collection-for-native-inserter branch from 52dc994 to ac90801 Compare July 14, 2022 23:21

dcalhoun removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jul 14, 2022

chore: Remove badge including text

fe810ca

The collection title is now rendered to make dark mode support easier.

dcalhoun commented Jul 15, 2022

View reviewed changes

dcalhoun requested a review from SiobhyB July 15, 2022 01:36

SiobhyB previously approved these changes Jul 15, 2022

View reviewed changes

Merge branch 'trunk' into feat/enable-jetpack-collection-for-native-i…

7aacb9c

…nserter

dcalhoun marked this pull request as ready for review July 15, 2022 15:35

dcalhoun added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jul 15, 2022

jeherve reviewed Jul 15, 2022

View reviewed changes

projects/plugins/jetpack/extensions/shared/block-category.native.js Outdated Show resolved Hide resolved

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 fellow Jetpack developers. Label will be renamed soon. labels Jul 15, 2022

fix: Translate Jetpack collection title

6073988

Co-authored-by: Jeremy Herve <[email protected]>

dcalhoun dismissed SiobhyB’s stale review via 6073988 July 15, 2022 15:43

jeherve approved these changes Jul 15, 2022

View reviewed changes

jeherve added this to the jetpack/11.2 milestone Jul 15, 2022

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! 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 Jul 15, 2022

jeherve enabled auto-merge (squash) July 15, 2022 15:44

jeherve merged commit 05e4647 into trunk Jul 15, 2022

jeherve deleted the feat/enable-jetpack-collection-for-native-inserter branch July 15, 2022 15:55

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jul 15, 2022

This was referenced Jul 21, 2022

Release 1.80.0 wordpress-mobile/gutenberg-mobile#5044

Merged

[RNMobile] Block Inserter: Limit Jetpack block collection to the WordPress app #25155

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable Jetpack collection for native block inserter #25092

feat: Enable Jetpack collection for native block inserter #25092

dcalhoun commented Jul 13, 2022 •

edited

Loading

matticbot commented Jul 13, 2022

github-actions bot commented Jul 13, 2022 •

edited

Loading

dcalhoun Jul 15, 2022

fluiddot Jul 15, 2022

SiobhyB Jul 15, 2022

dcalhoun Jul 15, 2022

fluiddot Jul 15, 2022

fluiddot Jul 15, 2022

dcalhoun Jul 15, 2022

fluiddot Jul 15, 2022

dcalhoun Jul 15, 2022 •

edited

Loading

fluiddot Jul 15, 2022

SiobhyB left a comment

dcalhoun commented Jul 15, 2022

jeherve commented Jul 15, 2022

github-actions bot commented Jul 15, 2022

feat: Enable Jetpack collection for native block inserter #25092

feat: Enable Jetpack collection for native block inserter #25092

Conversation

dcalhoun commented Jul 13, 2022 • edited Loading

Changes proposed in this Pull Request:

Other information:

Jetpack product discussion

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

Testing instructions:

matticbot commented Jul 13, 2022

github-actions bot commented Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcalhoun Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SiobhyB left a comment

Choose a reason for hiding this comment

dcalhoun commented Jul 15, 2022

jeherve commented Jul 15, 2022

github-actions bot commented Jul 15, 2022

dcalhoun commented Jul 13, 2022 •

edited

Loading

github-actions bot commented Jul 13, 2022 •

edited

Loading

dcalhoun Jul 15, 2022 •

edited

Loading