Add/value to active state videopress card #38812

CodeyGuyDylan · 2024-08-09T19:56:27Z

Proposed changes:

Add optional $days arg to function in VideoPress package that returns stats. This arg allows us to customize the days returned for featured stats without having to duplicate functionality in My Jetpack. It also doesn't break any current usages of the function as the argument is optional and defaults to the old value of 14 days
Add monthly views and watch-time to the active card state (if there is any)
Hide value section if the module is active but no views or watch time has been recorded

NOTE: This PR does not intend to handle adding trends, tooltip, or context switching between monthly and all-time views. Those will be handled in separate PRs

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:

On a Jurassic Ninja site, enable sandbox access and checkout this branch on the main Jetpack plugin.
Go to /wp-admin/options-general.php?page=companion_settings and enter your sandbox domain to enable sandboxed requests
Connect your user account
Add at least 1 video to your site
Go to My Jetpack and confirm you still see the inactive state showing 1 video you could be using VideoPress for
Now activate the VideoPress plugin with this branch. You probably won't have any video data on your test site as far as views or watch time goes, so you should see an active card with no content
To mock some randomized data, go to fbhepr%2Skers%2Sjcpbz%2Schoyvp.ncv%2Serfg%2Sjcpbz%2Qwfba%2Qraqcbvagf%2Spynff.jcpbz%2Qwfba%2Qncv%2Qfgngf%2Qivqrb%2Qcynlf%2Qi1%2Q1%2Qraqcbvag.cuc%3Se%3Q210o65nn%26zb%3Q1199%26sv%3Q37%2350-og and replace all those zeros with something like rand(0, 100). You can pick whatever numbers you'd like if you wanted to test out smaller or bigger numbers.
Now refresh My Jetpack and you should see some data

Play around with the random numbers to make sure they scale well with the compacting number/time formatting.
Additionally make sure they match designs here: p1HpG7-rb7-p2

Note: There are some pending questions as far as the design goes (p1HpG7-rb7-p2#comment-73251), so the CSS is pending, but the functionality should remain the same so I think it is ready to review

github-actions · 2024-08-09T19:57:34Z

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-active-state-videopress-card branch.

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

bin/jetpack-downloader test jetpack add/value-to-active-state-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-09T19:57:46Z

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.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
A. Team Review: add the "[Status] Needs Team Review" label
- For most changes, including minor cross-team impacts.
- Example: Updating a team-specific component or a small change to a shared library.
  B. Crew Review: add the "[Status] Needs Review" label
- For significant changes to core functionality.
- Example: Major updates to a shared library or complex features.
  C. Both: Start with Team, then request Crew
- For complex changes or when you need extra confidence.
- Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

CodeyGuyDylan · 2024-08-09T20:00:45Z

...ckages/my-jetpack/_inc/components/product-cards-section/protect-card/scan-threats-status.tsx

 import ShieldOff from './assets/shield-off.svg';
 import ShieldPartial from './assets/shield-partial.svg';
 import ShieldSuccess from './assets/shield-success.svg';
 import { InfoTooltip } from './info-tooltip';
 import { useProtectTooltipCopy } from './use-protect-tooltip-copy';
-import type { ReactElement, PropsWithChildren } from 'react';
+import type { PropsWithChildren, ReactElement } from 'react';


This was an accident but doesn't make a difference 😅

jboland88

Overall this tested well and is looking good, I had a few small comments and ran into some things that differed from the test instructions.

When setting up the test, after adding a video, I did not get the option to "Activate" VideoPress - I had "learn more". Not a blocker, but just noting that was different

After activating VideoPress through Jetpack settings, I did see the empty stats with 0s.

jboland88 · 2024-08-13T18:34:58Z

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

+	font-weight: 500;
+	margin-bottom: calc(var(--spacing-base) + 2px);
+	line-height: var(--font-title-small);
+	text-align: center;


Not sure the text-align is needed since the items are aligned with flex-start?

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

projects/packages/videopress/src/class-stats.php

jboland88 · 2024-08-13T18:47:12Z

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

@@ -315,9 +315,16 @@ public static function get_videopress_stats() {
 		}

 		$videopress_stats = new VideoPress_Stats();
+		$featured_stats   = $videopress_stats->get_featured_stats( 60 );


Wondering if we should add a transient cache for this call so we aren't calling it on each page load?

Yeah we could do that 🤔 I'll set it to 1 hour? Or would that be too long?

CodeyGuyDylan · 2024-08-13T19:01:49Z

When setting up the test, after adding a video, I did not get the option to "Activate" VideoPress - I had "learn more". Not a blocker, but just noting that was different

I assume this is because you don't meet the criteria for the product to be owned 🤔 If you didn't have the standalone plugin or a plan, and this showed up in the "Discover" section, based on the criteria we set before then I don't believe we should see an "Activate" 🤔 Let me know if my assumptions were wrong or if you think this should be fixed. It's also possible that a value on the backend of the videopress class needs to be updated

CodeyGuyDylan · 2024-08-14T18:48:35Z

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

@@ -29,6 +29,10 @@ const VideoPressValueSection: FC< VideoPressValueSectionProps > = ( { isPluginAc
 	const currentViews = featuredStats?.data?.views?.current;
 	const currentWatchTime = featuredStats?.data?.watch_time?.current;

+	if ( currentViews === undefined || currentWatchTime === undefined ) {


This is to handle the case where there was an error on the API call. 0's should be accepted if the site has videos

jboland88

One small nitpick, this LGTM

jboland88 · 2024-08-15T13:11:38Z

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

-
-	const MISSING_CONNECTION_NOTIFICATION_KEY = 'missing-connection';
+	const MISSING_CONNECTION_NOTIFICATION_KEY            = 'missing-connection';
+	const VIDEOPRESS_STATS_KEY                           = 'videopress-stats';


Nitpick - might be a good idea to add my-jetpack- to the front of this transient key just in case the videopress plugin decides to use a similar transient name at some point in the future.

Good idea, I'll add that

jboland88

Thanks for updating the transient name!

* Add value to active state of VideoPress card * changelog * Return videoCount if error on featured stats call * Update time format to accomodate years * Update definition of videopress active * Show stats if user has videos * Simplify logic of videopress value section * Left align protect headings per design * Update monthly views heading to 30-day views * Add transient * Specify transient key to my jetpack * Fixup project versions

github-actions bot added [Package] My Jetpack [Package] VideoPress labels Aug 9, 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 9, 2024

CodeyGuyDylan commented Aug 9, 2024

View reviewed changes

CodeyGuyDylan force-pushed the add/value-to-active-state-videopress-card branch from 536655e to b29bb5b Compare August 12, 2024 14:33

jboland88 requested changes Aug 13, 2024

View reviewed changes

CodeyGuyDylan force-pushed the add/value-to-active-state-videopress-card branch from b848419 to e97e266 Compare August 14, 2024 18:47

CodeyGuyDylan commented Aug 14, 2024

View reviewed changes

CodeyGuyDylan requested a review from jboland88 August 14, 2024 18:49

jboland88 previously approved these changes Aug 15, 2024

View reviewed changes

CodeyGuyDylan dismissed jboland88’s stale review via 510c733 August 15, 2024 15:05

CodeyGuyDylan requested a review from jboland88 August 15, 2024 15:05

CodeyGuyDylan added 11 commits August 15, 2024 09:35

Add value to active state of VideoPress card

c67562d

changelog

34c434e

Return videoCount if error on featured stats call

5e137be

Update time format to accomodate years

987ada5

Update definition of videopress active

8a2249f

Show stats if user has videos

d74c9c3

Simplify logic of videopress value section

fe99551

Left align protect headings per design

9f9c05b

Update monthly views heading to 30-day views

d5ec64d

Add transient

8a43805

Specify transient key to my jetpack

0bd3885

CodeyGuyDylan force-pushed the add/value-to-active-state-videopress-card branch from 510c733 to 0bd3885 Compare August 15, 2024 15:36

Fixup project versions

d7f38fb

jboland88 approved these changes Aug 15, 2024

View reviewed changes

CodeyGuyDylan merged commit 7015560 into trunk Aug 15, 2024
70 of 71 checks passed

CodeyGuyDylan deleted the add/value-to-active-state-videopress-card branch August 15, 2024 16:33

github-actions bot 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 Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add/value to active state videopress card #38812

Add/value to active state videopress card #38812

CodeyGuyDylan commented Aug 9, 2024 •

edited

Loading

github-actions bot commented Aug 9, 2024 •

edited

Loading

github-actions bot commented Aug 9, 2024 •

edited

Loading

CodeyGuyDylan Aug 9, 2024

jboland88 left a comment

jboland88 Aug 13, 2024

jboland88 Aug 13, 2024

CodeyGuyDylan Aug 14, 2024

CodeyGuyDylan commented Aug 13, 2024

CodeyGuyDylan Aug 14, 2024

jboland88 left a comment

jboland88 Aug 15, 2024

CodeyGuyDylan Aug 15, 2024

jboland88 left a comment

Add/value to active state videopress card #38812

Add/value to active state videopress card #38812

Conversation

CodeyGuyDylan commented Aug 9, 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 9, 2024 • edited Loading

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

CodeyGuyDylan Aug 9, 2024

Choose a reason for hiding this comment

jboland88 left a comment

Choose a reason for hiding this comment

jboland88 Aug 13, 2024

Choose a reason for hiding this comment

jboland88 Aug 13, 2024

Choose a reason for hiding this comment

CodeyGuyDylan Aug 14, 2024

Choose a reason for hiding this comment

CodeyGuyDylan commented Aug 13, 2024

CodeyGuyDylan Aug 14, 2024

Choose a reason for hiding this comment

jboland88 left a comment

Choose a reason for hiding this comment

jboland88 Aug 15, 2024

Choose a reason for hiding this comment

CodeyGuyDylan Aug 15, 2024

Choose a reason for hiding this comment

jboland88 left a comment

Choose a reason for hiding this comment

CodeyGuyDylan commented Aug 9, 2024 •

edited

Loading

github-actions bot commented Aug 9, 2024 •

edited

Loading

github-actions bot commented Aug 9, 2024 •

edited

Loading