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.

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

Plans: Disable longer plan term default in logged-in plans page #97129

Merged

Conversation

chriskmnds
Copy link
Contributor

Related to #96816, https://github.com/Automattic/martech/issues/3403
Related to p1732646141389969-slack-CV2TX2PAN and p1732648406144139-slack-CV2TX2PAN

Proposed Changes

This is a follow-up to #96816 where we disabled the term savings display for the logged-in plans page. That was only a part of the upcoming experiment. We disable the longer plan term default here as that's the main/core of the upcoming experiment, not the term savings display.

Why are these changes being made?

The same reasoning with #96816

Defaulting to longer term plans leads to a subpar UX experience in the logged in plans page. See this comment specifically p1732652464476219/1732648406.144139-slack-CV2TX2PAN.
It would also interfere with the Hide Lower Tiers Experiment pbxNRc-4gN-p2

Testing Instructions

  • Assign a treatment variation from pbxNRc-4tx-p2
  • Visit /plans/:site and ensure it doesn't default to a longer plan term, like 2-yearly or 3-yearly depending on variation

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
    • For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@chriskmnds chriskmnds requested a review from a team December 5, 2024 17:45
@chriskmnds chriskmnds self-assigned this Dec 5, 2024
@chriskmnds chriskmnds requested a review from oswian December 5, 2024 17:45
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 5, 2024
@chriskmnds chriskmnds requested a review from jeyip December 5, 2024 17:45
@chriskmnds
Copy link
Contributor Author

@jeyip I'm a bit confused how #96816 addressed not defaulting to the longer plan term when in logged-in pages. Wasn't that the intention? I am likely missing something from being away from this, but was it disabled without the changes in this PR? 🤔

@matticbot
Copy link
Contributor

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~64 bytes removed 📉 [gzipped])

name   parsed_size           gzip_size
plans       -179 B  (-0.0%)      -64 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@matticbot
Copy link
Contributor

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • notifications
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/plans-grid-longer-term-default-disable-loggedin on your sandbox.

@jeyip
Copy link
Contributor

jeyip commented Dec 5, 2024

@jeyip I'm a bit confused how #96816 addressed not defaulting to the longer plan term when in logged-in pages. Wasn't that the intention? I am likely missing something from being away from this, but was it disabled without the changes in this PR? 🤔

Ah yes this was on my radar of personal TODO's. "no longer need changes in my-sites/plans/main.jsx for longerPlanTermDefaultExperiment.term" Apologies for not communicating this more clearly in the PR or elsewhere.

I spotted the code some time after I merged #96816 and was planning to do a quick clean up among other things ( happiness heads up, stakeholder heads up, issue creation for bugs and potential refactors etc. ) before deploying to production.

I'll start elaborating on all of the remaining tasks I have in mind more clearly today. In the meantime, thanks for taking care of this Christos 🙏

@jeyip
Copy link
Contributor

jeyip commented Dec 5, 2024

Started updating https://github.com/Automattic/martech/issues/3403 with a more comprehensive list of remaining and optional tasks

@jeyip
Copy link
Contributor

jeyip commented Dec 5, 2024

Testing

Requirements

  • Assign a treatment variation from pbxNRc-4tx-p2
  • Visit /plans/:site and ensure it doesn't default to a longer plan term, like 2-yearly or 3-yearly depending on variation
  • Smoke test /start to ensure existing functionality with and without term savings UI and longer plan term defaults

Flows:

  • /start
  • /plans

Browsers

  • Chrome

@jeyip jeyip removed the request for review from oswian December 5, 2024 23:03
Copy link
Contributor

@jeyip jeyip left a comment

Choose a reason for hiding this comment

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

Tests well. On my way home from the coffee shop. Merging this when I arrive. Thanks again for drafting this Christos! 😄

@jeyip jeyip merged commit 72fa5c2 into trunk Dec 6, 2024
16 of 17 checks passed
@jeyip jeyip deleted the update/plans-grid-longer-term-default-disable-loggedin branch December 6, 2024 00:36
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants