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

Gutenboarding: get plan from URL param #41679

Merged
merged 2 commits into from
May 5, 2020

Conversation

razvanpapadopol
Copy link

@razvanpapadopol razvanpapadopol commented Apr 30, 2020

Changes proposed in this Pull Reques

  • Update path handling in Gutenboarding to include plan as an optional param.
  • So it should be possible to set the plan via URL argument like so: /new/PLAN_SLUG where the slug is beginner, personal, premium, business, or ecommerce.
  • The existing mechanism to set a Free / Premium Plan using the type of selected domain (free/paid) only if there isn't a valid plan param.

Testing instructions

  • Go to /new/beginner.
  • Free plan should be displayed in top-right corner.
  • After entering a site title and choosing a paid domain using Domain picker, Free plan should still be displayed.
  • Go to /new/business.
  • Business Plan should be displayed.
  • Go to /new/business/it.
  • Piano Business should be displayed (locale param should still work).
  • Go to /new/unknownplan/it.
  • A redirect to /new will happen but language should be set correctly.

Screenshot

Screenshot 2020-04-30 at 16 59 24

Fixes #41576

* Update path to include plan.
* Introduce plans.ts in gutenboarding lib.
* Update PlansButton to use lib/plans and path.
@razvanpapadopol razvanpapadopol added [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Goal] New Onboarding previously called Gutenboarding labels Apr 30, 2020
@razvanpapadopol razvanpapadopol requested a review from a team as a code owner April 30, 2020 14:50
@razvanpapadopol razvanpapadopol self-assigned this Apr 30, 2020
@matticbot
Copy link
Contributor

const plan = getPlanSlugByPath( planPath ) || ( hasPaidDomain ? defaultPaidPlan : freePlan );

/* translators: Button label where %s is the WordPress.com plan name (eg: Free, Personal, Premium, Business) */
const planLabel = sprintf( __( '%s Plan' ), getPlanTitle( plan ) );
Copy link
Member

@ramonjd ramonjd May 1, 2020

Choose a reason for hiding this comment

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

I'm not sure about this translation. It's not consistent with what we have over at WordPress.com/plans (both the logged-in page and logged-out marketing pages)

Many locales translate the plan names (Free, Business, and so on). I'm not arguing for one over the other, but for brand consistency, my guess is that we should describe the plans uniformly.

For example, take Japanese:

https://ja.wordpress.com/pricing/

and in Gutenboarding

Screen Shot 2020-05-01 at 10 56 03 am

Same for German, which only translations "Personal"

Screen Shot 2020-05-01 at 10 58 56 am

We already have 100% translation coverage for:

Business Plan
Personal Plan
Premium Plan
Strangely "eCommerce Plan" doesn't yet exist, but there's eCommerce

Copy link
Member

Choose a reason for hiding this comment

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

If I understood correctly, you suggested taking "Plan" word out of the header? I think it would be important to keep "Plan" word in the header for clarity ("Premium" what?). When plans are side-by-side on a grid, it's more clear that "personal" refers to a plan, but in the header it could refer e.g. to automatically assigned vertical. (@dubielzyk)

I think it's ok to merge this PR without changes to this I think and we can log follow-ups; generally we can bend the rest of the system for designs, vs try to bend designs for the code we happen to have.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's make the least amount of copy changes for now and I'll follow-up where we should override. We wanna be as consistent with the previous designs as possible

Copy link
Member

Choose a reason for hiding this comment

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

If I understood correctly, you suggested taking "Plan" word out of the header?

No, just highlighting that we need to be consistent.

We're introducing a new i18n string with __( '%s Plan' ).

Because of this, I think we need to make sure that we use the same nomenclature across our application including marketing pages, or that we follow the existing implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Fair point. I noted down consistently translating plans at #37665

@ramonjd
Copy link
Member

ramonjd commented May 1, 2020

Works as described. A really nice change! I left a note about translations, which I think should be consistent with our marketing copy over at WordPress.com/plans

@razvanpapadopol razvanpapadopol mentioned this pull request May 5, 2020
6 tasks
@dubielzyk dubielzyk self-requested a review May 5, 2020 11:57
@razvanpapadopol razvanpapadopol merged commit 0e54395 into master May 5, 2020
@razvanpapadopol razvanpapadopol deleted the add/gutenboarding-set-plan-using-url-param branch May 5, 2020 12:19
@johnHackworth johnHackworth mentioned this pull request May 7, 2020
46 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Goal] New Onboarding previously called Gutenboarding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gutenboarding: set plan via URL param
5 participants