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

Jump to bottom

My Jetpack: add slider for recommendations section in My Jetpack #39850

Merged
merged 10 commits into from
Nov 5, 2024
Merged

My Jetpack: add slider for recommendations section in My Jetpack #39850

merged 10 commits into from
Nov 5, 2024

Conversation

IanRamosC
Copy link
Contributor

@IanRamosC IanRamosC commented Oct 21, 2024 •
edited
Loading

Proposed changes:

  • This PR updates the recommendations sections in My Jetpack and displays 5 cards instead of 3. With the overflowing content, we decided to display this section as a slider, so the user can see more cards if they want to.
  • We also updated the Container component to accept ref as a prop
Screen.Recording.2024-10-23.at.16.49.11.mov

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

pbNhbs-bvL-p2#comment-23588

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

It adds two tracks events, one to each arrow button.

Testing instructions:

  • Spin up a JN site with Jetpack Beta pointing to this branch
  • Go to My Jetpack, click to Activate Jetpack with one click
  • After your site is connected, you'll see either a survey or the recommendations. If you see the survey, choose any option and submit it
  • When you see the survey, make sure the slider works as expected on different screen sizes

@IanRamosC IanRamosC self-assigned this Oct 21, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 21, 2024 •
edited
Loading

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/my-jetpack-recommendations-slider 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/my-jetpack-recommendations-slider

    bin/jetpack-downloader test jetpack-mu-wpcom-plugin add/my-jetpack-recommendations-slider

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 github-actions bot added [Package] My Jetpack [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. [Plugin] Boost A feature to speed up the site and improve performance. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Migration [Plugin] Protect A plugin with features to protect a site: brute force protection, security scanning, and a WAF. [Plugin] Search A plugin to add an instant search modal to your site to help visitors find content faster. [Plugin] Social Issues about the Jetpack Social plugin [Plugin] Starter Plugin [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. [Status] In Progress labels Oct 21, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 21, 2024 •
edited
Loading

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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. 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.
  3. Get at least one approval before merging.

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

Jetpack plugin:

The Jetpack plugin has different release cadences depending on the platform:

  • WordPress.com Simple releases happen semi-continuously (PCYsg-Jjm-p2).
  • WoA releases happen weekly.
  • Releases to self-hosted sites happen monthly. The next release is scheduled for none scheduled (scheduled code freeze on undefined).

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Backup plugin:

  • Next scheduled release: none scheduled.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Boost plugin:

  • Next scheduled release: none scheduled.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Search plugin:

  • Next scheduled release: none scheduled.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Social plugin:

  • Next scheduled release: none scheduled.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Starter Plugin plugin:

  • Next scheduled release: none scheduled.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Protect plugin:

  • Next scheduled release: none scheduled.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Videopress plugin:

  • Next scheduled release: none scheduled.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

Migration plugin:

  • Next scheduled release: none scheduled.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

@IanRamosC IanRamosC marked this pull request as ready for review October 23, 2024 20:14
robertsreberski
robertsreberski reviewed Oct 28, 2024
View reviewed changes
Copy link
Contributor

@robertsreberski robertsreberski left a comment

Choose a reason for hiding this comment

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

I love the attention to detail - animation of buttons is 🔥
I left couple of remarks, some of them might not be bugs, but features - feel free to treat them as opinions.

When there are only 2 cards, the first one gets stretched:
CleanShot 2024-10-28 at 20 41 35

I think I would expect cards to be displayed fully, or not at all. In this case, I would not expect the 4th card to be visible partially - instead, only 3 cards, and then on the scroll/arrow right, the 4th card would enter the viewport, and 1st card would leave. It's just my thought though - understand if the intentions were different. Maybe we could bring that up for design to check on? cc @ilonagl
CleanShot 2024-10-28 at 20 42 34

...s/packages/my-jetpack/_inc/data/evaluation-recommendations/use-evaluation-recommendations.ts Show resolved Hide resolved
...s/packages/my-jetpack/_inc/data/evaluation-recommendations/use-evaluation-recommendations.ts
// We filter out owned modules, and return top 3 recommendations
return recommendedModules?.filter( module => ! ownedProducts.includes( module ) ).slice( 0, 3 );
// We filter out owned modules, and return the top recommendations
return recommendedModules
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we will want to change the backend behavior to return only recommendations we actually want to show.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be addressed in a follow-up IMO.

@IanRamosC
Copy link
Contributor Author

Maybe we could bring that up for design to check on? cc @ilonagl

Thanks for the feedback.
I had pinged Ilona and we talked about the rationale behind that. In short, the arrows might not be enough to draw attention to the cards beyond the viewport. But I'm open to update the styles.

@ilonagl
Copy link

ilonagl commented Oct 29, 2024

Thanks for the feedback.
I had pinged Ilona and we talked about the rationale behind that. In short, the arrows might not be enough to draw attention to the cards beyond the viewport. But I'm open to update the styles.

Yes, we discussed this with @IanRamosC and thought it would help users understand that there are more cards available.

@IanRamosC IanRamosC force-pushed the add/my-jetpack-recommendations-slider branch from b57e9e9 to 2b9479c Compare November 3, 2024 03:41
CodeyGuyDylan
CodeyGuyDylan previously requested changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@CodeyGuyDylan CodeyGuyDylan left a comment

Choose a reason for hiding this comment

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

This looks pretty good! I had a few comments in the code and an issue I found with mobile views. After those are addressed I think we should be good to ship

On mobile views, the slider can get into a weird state, see below:

scrolling-issue.mov

I think the easiest fix for this would be to make the maximum width of the cards the width of the screen minus padding/margin. Looking at the My Jetpack cards, they seem to have this:

image

projects/packages/my-jetpack/_inc/components/evaluation-recommendations/index.tsx Outdated Show resolved Hide resolved
projects/packages/my-jetpack/_inc/components/evaluation-recommendations/index.tsx
const { recordEvent } = useAnalytics();
const { recommendedModules, redoEvaluation, removeEvaluationResult } =
useEvaluationRecommendations();
const [ isAtStart, setIsAtStart ] = useState( true );
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure it's necessary to extract it in this PR, but I think it would be nice if all this scroll stuff was in a component called something like <ScrollContainer />. Not sure how easy/realistic it is to make it reusable like that but I think that would be best case scenario 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's a good idea to make that reusable, but I don't think that should be the case for now, since we are not really using it anywhere else (and I can't imagine another use of it in the foreseeable future).

projects/packages/my-jetpack/_inc/components/evaluation-recommendations/index.tsx Outdated Show resolved Hide resolved
elliottprogrammer
elliottprogrammer reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@elliottprogrammer elliottprogrammer left a comment

Choose a reason for hiding this comment

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

Tested functionality. It works and looks great! 🙌
Other than the weird state it can get into, mentioned by Dylan above (although which I did not notice during my testing), everything worked great and looks great from a functionality point of view. 👍
And same with the code, other than the comments by Dylan, the code looks all good to me!
LGTM! 👍 But I'll defer the final approval to @CodeyGuyDylan since he's requested some small changes. 🙂

elliottprogrammer
elliottprogrammer approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@elliottprogrammer elliottprogrammer left a comment

Choose a reason for hiding this comment

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

Cool, the weird scrolling state that was occurring in mobile view is fixed and works great now! 💪
Everything LGTM! 👍

@CodeyGuyDylan CodeyGuyDylan dismissed their stale review November 4, 2024 23:26

Updated review

@IanRamosC IanRamosC merged commit 71a65ee into trunk Nov 5, 2024
75 checks passed
@IanRamosC IanRamosC deleted the add/my-jetpack-recommendations-slider branch November 5, 2024 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertsreberski robertsreberski robertsreberski left review comments

@elliottprogrammer elliottprogrammer elliottprogrammer approved these changes

@CodeyGuyDylan CodeyGuyDylan CodeyGuyDylan left review comments

Assignees

@IanRamosC IanRamosC

Labels
[JS Package] Components [Package] My Jetpack [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. [Plugin] Boost A feature to speed up the site and improve performance. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Migration [Plugin] Protect A plugin with features to protect a site: brute force protection, security scanning, and a WAF. [Plugin] Search A plugin to add an instant search modal to your site to help visitors find content faster. [Plugin] Social Issues about the Jetpack Social plugin [Plugin] Starter Plugin [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. RNA
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@IanRamosC @ilonagl @robertsreberski @elliottprogrammer @CodeyGuyDylan