-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Focused Launch: PlanDetails View #47373
Changes from 8 commits
c2f6212
bc5b97e
db82c09
736b6ce
565027d
e067ec0
dd18974
02997af
00bf30e
ce885aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,22 +3,80 @@ | |
/** | ||
* External dependencies | ||
*/ | ||
import React from 'react'; | ||
import { Link } from 'react-router-dom'; | ||
import * as React from 'react'; | ||
import { useSelect, useDispatch } from '@wordpress/data'; | ||
import { __ } from '@wordpress/i18n'; | ||
import { Plans } from '@automattic/data-stores'; | ||
import PlansGrid from '@automattic/plans-grid'; | ||
import { Title, SubTitle } from '@automattic/onboarding'; | ||
import { useHistory } from 'react-router-dom'; | ||
import { BackButton } from '@automattic/onboarding'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { Route } from '../route'; | ||
import { LAUNCH_STORE } from '../../stores'; | ||
|
||
import './style.scss'; | ||
|
||
const PlanDetails: React.FunctionComponent = () => { | ||
const domain = useSelect( ( select ) => select( LAUNCH_STORE ).getSelectedDomain() ); | ||
const selectedPlan = useSelect( ( select ) => select( LAUNCH_STORE ).getSelectedPlan() ); | ||
const history = useHistory(); | ||
|
||
const { updatePlan } = useDispatch( LAUNCH_STORE ); | ||
|
||
const hasPaidDomain = domain && ! domain.is_free; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If If you changed Not a blocker though, as it'll be coerced into a falsely value anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great idea but Typescript won't like because |
||
|
||
const handleSelect = ( planSlug: Plans.PlanSlug ) => { | ||
updatePlan( planSlug ); | ||
history.goBack(); | ||
}; | ||
|
||
const goBackToSummary = () => { | ||
history.goBack(); | ||
}; | ||
|
||
return ( | ||
<div> | ||
<Link to={ Route.Summary }>{ __( 'Go back', __i18n_text_domain__ ) }</Link> | ||
<p>{ __( 'Select a plan', __i18n_text_domain__ ) }</p> | ||
<BackButton onClick={ goBackToSummary }>{ __( 'Go back', __i18n_text_domain__ ) }</BackButton> | ||
<div className="focused-launch-plan-details__header"> | ||
<div> | ||
<Title>{ __( 'Select a plan', __i18n_text_domain__ ) }</Title> | ||
<SubTitle> | ||
{ __( | ||
"There's no risk, you can cancel for a full refund within 30 days.", | ||
__i18n_text_domain__ | ||
) } | ||
</SubTitle> | ||
</div> | ||
</div> | ||
<div className="focused-launch-plan-details__body"> | ||
<PlansGrid | ||
currentDomain={ domain } | ||
onPlanSelect={ handleSelect } | ||
currentPlan={ selectedPlan } | ||
onPickDomainClick={ goBackToSummary } | ||
customTagLines={ { | ||
free_plan: __( 'Best for getting started', __i18n_text_domain__ ), | ||
'business-bundle': __( 'Best for small businesses', __i18n_text_domain__ ), | ||
} } | ||
showPlanTaglines | ||
popularBadgeVariation="NEXT_TO_NAME" | ||
disabledPlans={ | ||
hasPaidDomain | ||
? { | ||
[ Plans.PLAN_FREE ]: __( | ||
'Not available with custom domain', | ||
__i18n_text_domain__ | ||
), | ||
} | ||
: undefined | ||
} | ||
CTAVariation="FULL_WIDTH" | ||
locale="user" | ||
/> | ||
</div> | ||
</div> | ||
); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,8 @@ import debugFactory from 'debug'; | |
import PlansTable from '../plans-table'; | ||
import PlansAccordion from '../plans-accordion'; | ||
import PlansDetails from '../plans-details'; | ||
import type { CTAVariation, CustomTagLinesMap, PopularBadgeVariation } from '../plans-table/types'; | ||
export type { CTAVariation, CustomTagLinesMap, PopularBadgeVariation } from '../plans-table/types'; | ||
|
||
/** | ||
* Style dependencies | ||
|
@@ -34,6 +36,10 @@ export interface Props { | |
disabledPlans?: { [ planSlug: string ]: string }; | ||
isAccordion?: boolean; | ||
locale: string; | ||
showPlanTaglines?: boolean; | ||
CTAVariation?: CTAVariation; | ||
popularBadgeVariation?: PopularBadgeVariation; | ||
customTagLines?: CustomTagLinesMap; | ||
} | ||
|
||
const PlansGrid: React.FunctionComponent< Props > = ( { | ||
|
@@ -46,6 +52,10 @@ const PlansGrid: React.FunctionComponent< Props > = ( { | |
disabledPlans, | ||
isAccordion, | ||
locale, | ||
showPlanTaglines = false, | ||
CTAVariation = 'NORMAL', | ||
popularBadgeVariation = 'ON_TOP', | ||
Comment on lines
+56
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to maintain another variation of the PlansGrid in a non-accordion mode. However, it's non blocking for this PR. Just pinging @ollierozdarz to confirm so we can clean up later. Here is the old table version with badge on top and CTA non full-width. |
||
customTagLines, | ||
} ) => { | ||
isAccordion && debug( 'PlansGrid accordion version is active' ); | ||
|
||
|
@@ -67,12 +77,16 @@ const PlansGrid: React.FunctionComponent< Props > = ( { | |
></PlansAccordion> | ||
) : ( | ||
<PlansTable | ||
popularBadgeVariation={ popularBadgeVariation } | ||
CTAVariation={ CTAVariation } | ||
selectedPlanSlug={ currentPlan?.storeSlug ?? '' } | ||
onPlanSelect={ onPlanSelect } | ||
customTagLines={ customTagLines } | ||
currentDomain={ currentDomain } | ||
onPickDomainClick={ onPickDomainClick } | ||
disabledPlans={ disabledPlans } | ||
locale={ locale } | ||
showTaglines={ showPlanTaglines } | ||
></PlansTable> | ||
) } | ||
</div> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what are the benefits of
import * as React
overimport React
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this. Looks like it's being used for the functional component typings but not sure the benefit of
*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would work anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we used this way of importing for a while for performance reasons. I believe now we have webpack configured so this isn't necessary anymore. Pinging @sirreal to confirm though 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do
import React...
you needallowSyntheticDefaultImports = true
in tsconfig.import * as React
is more semantically accurate and makes TypeScript happy. But don't quote me on this. I think (not sure) I saw @sirreal do it, and he is my TypeScript prophet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some perf concerns and I don't know whether they're relevant. Either way, I think we have conclusive evidence that
import * as React from 'react';
is "right": facebook/react#18102We could add a lint rule for this and/or codemod it, but I don't know whether there is significant impact beyond consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation! I learnt something new
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this is right. Note, this isn't really about TypeScript but about ECMAScript modules which are quirky. I wrote a bit here (p4TIVU-8Lf-p2).