Add value to inactive state of VideoPress #38748

CodeyGuyDylan · 2024-08-06T18:33:26Z

Proposed changes:

When the VideoPress card is inactive but the user has videos, show how many videos they could improve with VideoPress
Fix some type errors in both the PHP and React side
Import VideoPress package into My Jetpack to use later for VideoPress stats

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?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

P2: p1HpG7-rb7-p2

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

No

Testing instructions:

Checkout this branch onto the Jetpack Backup standalone plugin (any plugin other than the main Jetpack plugin or VideoPress
Go to My Jetpack and make sure the VideoPress card looks inactive and no errors are thrown
Now add a video to your site (or multiple videos). Go back to My Jetpack and you should see the number of videos plus an updated description
If you install the VideoPress (or Jetpack) plugin, you should see the same thing if it is not active
Activate the plugin and you should see the regular description (this will be updated in a future PR)
Now remove the videos. You should see some description letting them know they can start by uploading their first video
Now in the console, log out myJetpackInitialState.videopress and make sure the featuredStats are being loaded. They're probably all 0 since your site is new, but it's enough for now that the data is loading

github-actions · 2024-08-06T18:34:16Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the add/value-to-videopress-card branch.
- For jetpack-mu-wpcom changes, also add define( 'JETPACK_MU_WPCOM_LOAD_VIA_BETA_PLUGIN', true ); to your wp-config.php file.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack add/value-to-videopress-card

bin/jetpack-downloader test jetpack-mu-wpcom-plugin add/value-to-videopress-card

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions · 2024-08-06T18:34:36Z

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.
✅ 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 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 reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

robertsreberski

Looks good 😄 I've tried to give feedback / sometimes opinions through the code, you can apply when you feel it's appropriate!

robertsreberski · 2024-08-07T15:23:16Z

projects/packages/my-jetpack/_inc/components/connected-product-card/index.tsx

@@ -85,7 +87,7 @@ const ConnectedProductCard: FC< ConnectedProductCardProps > = ( {

 	const DefaultDescription = () => {
 		// Replace the last space with a non-breaking space to prevent widows
-		const cardDescription = defaultDescription.replace( /\s(?=[^\s]*$)/, '\u00A0' );
+		const cardDescription = preventWidows( defaultDescription );


Reducing technical debt 💪 great!

robertsreberski · 2024-08-07T15:26:44Z

projects/packages/my-jetpack/_inc/components/product-cards-section/types.ts

@@ -0,0 +1,6 @@
+import type { FC } from 'react';
+
+export type ProductCardType = FC< {


Idea for name: ProductCardComponent/ProductCardFC? I like the unification!

ProductCardComponent sounds better to me, thanks!

robertsreberski · 2024-08-07T15:27:47Z

projects/packages/my-jetpack/_inc/components/product-cards-section/videopress-card/index.tsx

+import './style.scss';
+
+const VideopressCard: ProductCardType = ( { admin } ) => {
+	const slug = 'videopress';


We could consider putting static variables outside of component scope?

I don't think it matters too much but we definitely could do that 🤔 And it'd be easy to do it now before we get too far into the product card projects

robertsreberski · 2024-08-07T15:30:07Z

projects/packages/my-jetpack/_inc/components/product-cards-section/videopress-card/index.tsx

+			slug={ slug }
+			showMenu
+			admin={ admin }
+			Description={ descriptionText ? Description : null }


Maybe Description variable could be already null if descriptionText is not provided?

I tried this but Description is a function so it does not return null until after it is passed to the function. I think we need to do it inline here 🤔

Yeah, my bad! I thought we can do something like this useCallback( descriptionText ? () => <Description /> : null, [ descriptionText ] ); But I see we cannot.

robertsreberski · 2024-08-07T15:32:13Z

projects/packages/my-jetpack/_inc/components/product-cards-section/videopress-card/style.scss

+p.description {
+    color: var( --jp-gray-70 );
+    font-size: var( --font-body-small );
+    margin: 0 0 1rem 0;


Suggested change

margin: 0 0 1rem 0;

margin: 0 0 1rem;

robertsreberski · 2024-08-07T15:36:41Z

...y-jetpack/_inc/components/product-cards-section/videopress-card/videopress-value-section.tsx

+		);
+	}
+
+	return <></>;


Why not null?

It's just a habit to return some sort of component, shouldn't make any difference but I'll update it to null 😄

robertsreberski · 2024-08-07T15:37:31Z

...y-jetpack/_inc/components/product-cards-section/videopress-card/videopress-value-section.tsx

+const VideoPressValueSection: FC< VideoPressValueSectionProps > = ( { isPluginActive, data } ) => {
+	if ( ! isPluginActive && data.videoCount ) {
+		return (
+			<div className={ 'videopress-card__video-count' }>


Suggested change

<div className={ 'videopress-card__video-count' }>

<div className="videopress-card__video-count">

Nice catch 😆

robertsreberski · 2024-08-07T15:37:56Z

...y-jetpack/_inc/components/product-cards-section/videopress-card/videopress-value-section.tsx

+	if ( ! isPluginActive && data.videoCount ) {
+		return (
+			<div className={ 'videopress-card__video-count' }>
+				<span>{ data.videoCount }</span>


Do we need <span> tag here?

Hmmm I prefer it rather than just having text straight in the <div> element 🤔 Does that logic sound decent to you?

Why cannot we have <span className="videopress-card__video-count">?

The logic sounds good to me, thanks for explaining!

That could work style-wise 🤔

I think I just like container enclosed items such as this. But I think I agree with you that it is not necessary in this case

robertsreberski · 2024-08-07T15:44:16Z

projects/packages/my-jetpack/src/class-rest-product-data.php

 	}

 	/**
 	 * This will collect a count of all the items that could be backed up
 	 * This is used to show what backup could be doing if it is not enabled
 	 *
-	 * @return array
+	 * @return WP_Error|\WP_REST_Response


Usually, these kind of changes to php docs "irritate" phan 😅 maybe the static check will pass if you revert it / change to WP_Error|array?

Hmm this was throwing a warning since array is not what is returned 🤔 But it's been this way for a while and if it's going to cause some phan shenanigans then maybe I'll just revert that

It seems like that didn't fix the phan errors 😓 Maybe I'll just move it back to array and see if that helps

I reverted it back to array and I'm still getting the PHAN errors 😢

I saw your conversation in jetpack devs channel, glad you got it fixed!

robertsreberski · 2024-08-07T15:46:04Z

projects/packages/my-jetpack/src/class-initializer.php

@@ -277,6 +278,10 @@ public static function enqueue_scripts() {
 						array( 'blocked_logins' => (int) get_site_option( 'jetpack_protect_blocked_attempts', 0 ) )
 					),
 				),
+				'videopress'             => array(
+					'featuredStats' => self::get_videopress_stats(),
+					'videoCount'    => array_sum( (array) wp_count_attachments( 'video' ) ),


we could include that code in self::get_videopress_stats() function?
I think it's a nice practice not to perform any transformations in the array definition itself.

That's a good note, I'll get that moved

robertsreberski

Thanks for the changes here! One more portion of minor feedback.

robertsreberski · 2024-08-09T10:16:07Z

projects/packages/my-jetpack/_inc/components/connected-product-card/index.tsx

 	isDataLoading?: boolean;
 	Description?: FC;
 	additionalActions?: AdditionalAction[];
 	secondaryAction?: SecondaryAction;
 	upgradeInInterstitial?: boolean;
-	primaryActionOverride?: AdditionalAction;
+	primaryActionOverride?: { [ key: string ]: AdditionalAction };


Suggested change

primaryActionOverride?: { [ key: string ]: AdditionalAction };

primaryActionOverride?: Record< strong, AdditionalAction >

robertsreberski · 2024-08-09T10:18:09Z

projects/packages/my-jetpack/_inc/components/product-cards-section/index.tsx

 	// We don't have a card for Security or Extras, and scan is displayed as protect.
-	Exclude< JetpackModule, 'extras' | 'scan' | 'security' >,
+	Exclude< JetpackModule, 'extras' | 'scan' | 'security' | 'ai' >,


Let's update the comment above as well. Currently it's a bit confusing because in a way "ai" card is displayed (I'm aware it's about reducing redundancy of "jetpack-ai" and "ai" - but I might forget it in ~3 months 😅)

robertsreberski · 2024-08-09T10:20:45Z

projects/packages/my-jetpack/_inc/components/product-cards-section/protect-card/index.tsx

-	);
-};
+const ProtectCard: ProductCardComponent = props => (
+	<ProductCard { ...props } slug={ PRODUCT_SLUGS.PROTECT } upgradeInInterstitial={ true }>


Suggested change

<ProductCard { ...props } slug={ PRODUCT_SLUGS.PROTECT } upgradeInInterstitial={ true }>

<ProductCard { ...props } slug={ PRODUCT_SLUGS.PROTECT } upgradeInInterstitial>

robertsreberski · 2024-08-09T10:27:14Z

projects/packages/my-jetpack/_inc/components/product-cards-section/videopress-card/index.tsx

+			slug={ slug }
+			showMenu
+			admin={ admin }
+			Description={ descriptionText ? Description : null }


Yeah, my bad! I thought we can do something like this useCallback( descriptionText ? () => <Description /> : null, [ descriptionText ] ); But I see we cannot.

robertsreberski · 2024-08-09T10:28:12Z

...y-jetpack/_inc/components/product-cards-section/videopress-card/videopress-value-section.tsx

+	if ( ! isPluginActive && data.videoCount ) {
+		return (
+			<div className={ 'videopress-card__video-count' }>
+				<span>{ data.videoCount }</span>


Why cannot we have <span className="videopress-card__video-count">?

The logic sounds good to me, thanks for explaining!

robertsreberski · 2024-08-09T10:28:52Z

projects/packages/my-jetpack/_inc/data/constants.ts

+	PROTECT: 'protect',
+	VIDEOPRESS: 'videopress',
+	STATS: 'stats',
+} satisfies Record< string, JetpackModule >;


Learned new Typescript syntax today, thanks 😁

I also learned that yesterday 😅 It seems like the best way to enforce the value of the object, but infer the types of the key. That way when you're using this in a typescript file, you will be limited to certain keys and still be able to see the value of the key 😄

The other implementations I tried either had the key as a generic "string" type which is problematic as you could use any key you wanted without a typescript error, or the key would be defined but the value would be a generic "string" type and I wanted it to be linked to the JetpackModules type

robertsreberski · 2024-08-09T10:29:35Z

projects/packages/my-jetpack/src/class-rest-product-data.php

 	}

 	/**
 	 * This will collect a count of all the items that could be backed up
 	 * This is used to show what backup could be doing if it is not enabled
 	 *
-	 * @return array
+	 * @return WP_Error|\WP_REST_Response


I saw your conversation in jetpack devs channel, glad you got it fixed!

robertsreberski · 2024-08-09T10:31:08Z

projects/packages/my-jetpack/_inc/utils/prevent-widows.ts

+	).part;
+};
+
+export default preventWidows;


Thanks for extending logic of that function! What do you think about adding a reference to the source in the comment in this file?

That's a good note, I'll add that

CodeyGuyDylan changed the title ~~Add/value to videopress card~~ Add value to inactive state of VideoPress Aug 6, 2024

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 Aug 6, 2024

CodeyGuyDylan force-pushed the add/value-to-videopress-card branch from faed468 to da0bb73 Compare August 6, 2024 18:52

robertsreberski reviewed Aug 7, 2024

View reviewed changes

CodeyGuyDylan force-pushed the add/value-to-videopress-card branch from bb386f5 to 5bc5af6 Compare August 7, 2024 21:44

CodeyGuyDylan added 2 commits August 8, 2024 09:07

Update inactive state for videopress card

de372d3

Update composer for plugins using My Jetpack

2547376

CodeyGuyDylan added 9 commits August 8, 2024 09:07

Introduct product slug constants

c4223dc

Return null for descripion if description text is null

9896c0c

optimize scss margin

b8b6ff2

Improve implementation of prevent widows

8e79bdb

Remove unnecessary brackets

866075e

Revert Description back to inline ternary

f6f1056

Return video count with rest of videopress stats

4dd7d3e

Revert php typing change

c2d5208

Fix types

ee0ce3e

CodeyGuyDylan force-pushed the add/value-to-videopress-card branch from a79438a to ee0ce3e Compare August 8, 2024 15:19

CodeyGuyDylan added 8 commits August 8, 2024 09:26

Remove type casting

023ec96

Re-add type casting

07c1aae

Remove composer update

83a13dc

Add endline

8c96cdb

Update phan

24e5b5b

Fix some php typing

db9e4c3

Fix type on jetpack connection function

680a968

changelog

983ba18

github-actions bot added the [Package] Connection label Aug 8, 2024

Fixup changelogger

978528e

robertsreberski reviewed Aug 9, 2024

View reviewed changes

CodeyGuyDylan added 5 commits August 9, 2024 08:38

Update primary action type

a6a16fa

Clarify why we are exluding ai in display card types

64a0ec0

Shorten boolean prop

3838a41

Simplify video count markup

80cffa0

Add comment sourcing the prevent widows function

d889837

robertsreberski approved these changes Aug 9, 2024

View reviewed changes

CodeyGuyDylan merged commit 6e45519 into trunk Aug 9, 2024
72 of 73 checks passed

CodeyGuyDylan deleted the add/value-to-videopress-card branch August 9, 2024 15:12

github-actions bot removed the [Status] Needs Team Review label Aug 9, 2024

		@@ -0,0 +1,6 @@
		import type { FC } from 'react';

		export type ProductCardType = FC< {

	<div className={ 'videopress-card__video-count' }>
	<div className="videopress-card__video-count">

	primaryActionOverride?: { [ key: string ]: AdditionalAction };
	primaryActionOverride?: Record< strong, AdditionalAction >

	<ProductCard { ...props } slug={ PRODUCT_SLUGS.PROTECT } upgradeInInterstitial={ true }>
	<ProductCard { ...props } slug={ PRODUCT_SLUGS.PROTECT } upgradeInInterstitial>

Add value to inactive state of VideoPress #38748

Add value to inactive state of VideoPress #38748

Conversation

CodeyGuyDylan commented Aug 6, 2024 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

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

Testing instructions:

github-actions bot commented Aug 6, 2024 • edited Loading

github-actions bot commented Aug 6, 2024 • edited Loading

robertsreberski left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertsreberski left a 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

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

CodeyGuyDylan commented Aug 6, 2024 •

edited

Loading

github-actions bot commented Aug 6, 2024 •

edited

Loading

github-actions bot commented Aug 6, 2024 •

edited

Loading