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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions client/landing/gutenboarding/components/plans-button/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ import { useI18n } from '@automattic/react-i18n';
* Internal dependencies
*/
import JetpackLogo from 'components/jetpack-logo'; // @TODO: extract to @automattic package
import * as plans from 'lib/plans/constants';
import { getPlan } from 'lib/plans';
import { STORE_KEY as ONBOARD_STORE } from '../../stores/onboard';
import { freePlan, defaultPaidPlan, getPlanSlugByPath, getPlanTitle } from '../../lib/plans';
import { usePlanRouteParam } from '../../path';

/**
* Style dependencies
Expand All @@ -23,16 +23,16 @@ import './style.scss';

const PlansButton: React.FunctionComponent< Button.ButtonProps > = ( { ...buttonProps } ) => {
const { __ } = useI18n();
const hasPaidDomain = useSelect( ( select ) => select( ONBOARD_STORE ).hasPaidDomain() );

// mobile first to match SCSS media query https://github.com/Automattic/wp-calypso/pull/41471#discussion_r415678275
const isDesktop = useViewportMatch( 'mobile', '>=' );

const planLabel = sprintf(
/* translators: Button label where %s is the WordPress.com plan name (eg: Free, Personal, Premium, Business) */
__( '%s Plan' ),
getPlan( hasPaidDomain ? plans.PLAN_PREMIUM : plans.PLAN_FREE ).getTitle()
);
const hasPaidDomain = useSelect( ( select ) => select( ONBOARD_STORE ).hasPaidDomain() );
const planPath = usePlanRouteParam();
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


return (
<Button disabled label={ __( planLabel ) } className="plans-button" { ...buttonProps }>
Expand Down
26 changes: 26 additions & 0 deletions client/landing/gutenboarding/lib/plans.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* Internal dependencies
*/
import * as plans from 'lib/plans/constants';
import { getPlan, getPlanPath } from 'lib/plans';

const supportedPlans = [
plans.PLAN_FREE,
plans.PLAN_PERSONAL,
plans.PLAN_PREMIUM,
plans.PLAN_BUSINESS,
plans.PLAN_ECOMMERCE,
];

export const freePlan = plans.PLAN_FREE;
export const defaultPaidPlan = plans.PLAN_PREMIUM;

export const supportedPlansPaths: string[] = supportedPlans.map( getPlanPath );

export function getPlanSlugByPath( path?: string ): string | undefined {
return supportedPlans.find( ( plan ) => getPlanPath( plan ) === path );
}

export function getPlanTitle( planSlug: string ): string {
return getPlan( planSlug ).getTitle();
}
29 changes: 24 additions & 5 deletions client/landing/gutenboarding/path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ import { generatePath, useLocation, useRouteMatch } from 'react-router-dom';
import { getLanguageRouteParam } from '../../lib/i18n-utils';
import { ValuesType } from 'utility-types';

/**
* Internal dependencies
*/
import { supportedPlansPaths } from './lib/plans';

// The first step (IntentGathering), which is found at the root route (/), is set as
// `undefined`, as that's what matching our `path` pattern against a route with no explicit
// step fragment will return.
Expand All @@ -21,31 +26,40 @@ export const Step = {
// We remove falsey `steps` with `.filter( Boolean )` as they'd mess up our |-separated route pattern.
export const steps = Object.values( Step ).filter( Boolean );

// We add back the possibility of an empty step fragment through the `?` question mark at the end of that fragment.
export const path = `/:step(${ steps.join( '|' ) })?/${ getLanguageRouteParam() }`;
const routeFragments = {
// We add the possibility of an empty step fragment through the `?` question mark at the end of that fragment.
step: `:step(${ steps.join( '|' ) })?`,
plan: `:plan(${ supportedPlansPaths.join( '|' ) })?`,
lang: getLanguageRouteParam(),
};

export const path = [ '', ...Object.values( routeFragments ) ].join( '/' );

export type StepType = ValuesType< typeof Step >;
export type StepNameType = keyof typeof Step;

export function usePath() {
const langParam = useLangRouteParam();
const planParam = usePlanRouteParam();

return ( step?: StepType, lang?: string ) => {
return ( step?: StepType, lang?: string, plan?: string ) => {
// When lang is null, remove lang.
// When lang is empty or undefined, get lang from route param.
lang = lang === null ? '' : lang || langParam;
plan = plan === null ? '' : plan || planParam;

if ( ! step && ! lang ) {
if ( ! step && ! lang && ! plan ) {
return '/';
}

try {
return generatePath( path, {
step,
plan,
lang,
} );
} catch {
// If we get an invalid lang, `generatePath` throws a TypeError.
// If we get an invalid lang or plan, `generatePath` throws a TypeError.
return generatePath( path, { step } );
}
};
Expand All @@ -61,6 +75,11 @@ export function useStepRouteParam() {
return match?.params.step as StepType;
}

export function usePlanRouteParam() {
const match = useRouteMatch< { plan?: string } >( path );
return match?.params.plan;
}

export function useCurrentStep() {
const stepRouteParam = useStepRouteParam();
return findKey( Step, ( step ) => step === stepRouteParam ) as StepNameType;
Expand Down