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

feat: Enable Jetpack collection for native block inserter #25092

Merged
merged 6 commits into from
Jul 15, 2022
Merged

feat: Enable Jetpack collection for native block inserter #25092

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
577ac7b
[not verified] feat: Enable Jetpack collection for native block inserter
dcalhoun Jul 11, 2022
e6d5b75
[not verified] changelog
dcalhoun Jul 13, 2022
ac90801
[not verified] chore: Update JetpackPoweredBadge asset
dcalhoun Jul 14, 2022
fe810ca
chore: Remove badge including text
dcalhoun Jul 15, 2022
7aacb9c
Merge branch 'trunk' into feat/enable-jetpack-collection-for-native-i…
dcalhoun Jul 15, 2022
6073988
fix: Translate Jetpack collection title
dcalhoun Jul 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions projects/plugins/jetpack/changelog/enable-jetpack-collection-for-native-inserter
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: enhancement

Enable Jetpack block collection for the native editor block inserter.
28 changes: 28 additions & 0 deletions projects/plugins/jetpack/extensions/shared/block-category.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { getCategories, setCategories, registerBlockCollection } from '@wordpress/blocks';
import { __ } from '@wordpress/i18n';
import { JetpackLogo } from './icons';

registerBlockCollection( 'jetpack', {
title: 'Jetpack powered',
dcalhoun marked this conversation as resolved.
Show resolved Hide resolved
icon: <JetpackLogo />,
} );
Copy link
Member Author

Choose a reason for hiding this comment

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

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

  1. The isWpCom conditional is removed (this check relies upon window and does not work for the native editor).
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link

Choose a reason for hiding this comment

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

+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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

+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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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 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. 👍🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

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 👍 .

Copy link
Member Author

@dcalhoun dcalhoun Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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'.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


setCategories( [
...getCategories().filter( ( { slug } ) => slug !== 'earn' ),
// Add a Earn block category
{
slug: 'earn',
title: __( 'Earn', 'jetpack' ),
icon: <JetpackLogo />,
},
] );

setCategories( [
...getCategories().filter( ( { slug } ) => slug !== 'grow' ),
// Add a Grow block category
{
slug: 'grow',
title: __( 'Grow', 'jetpack' ),
icon: <JetpackLogo />,
},
] );