Focused Launch: PlanDetails View by tjcafferkey · Pull Request #47373 · Automattic/wp-calypso

tjcafferkey · 2020-11-12T14:22:55Z

Changes proposed in this Pull Request

Implements the detailed plans step in Focused Launch

Testing instructions

In the project root, run yarn && yarn start. In a separate terminal window, run cd apps/editing-toolkit && yarn dev --sync.
Sandbox YOUR_SITE.wordpress.com.
Visit https://YOUR_SITE.wordpress.com/wp-admin/post-new.php.
In console, type wp.data.dispatch('automattic/launch' ).openFocusedLaunch().
Click "View all plans".
You should see the plans grid exactly as in the Figma file.

Fixes #46867

matticbot · 2020-11-12T14:22:57Z

Test live: https://calypso.live/?branch=update/focused-launch-all-plans-view

matticbot · 2020-11-16T14:00:49Z

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

App Entrypoints (~480 bytes added 📈 [gzipped])

Details

name                 parsed_size           gzip_size
entry-gutenboarding      +2437 B  (+0.1%)     +480 B  (+0.1%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~122 bytes added 📈 [gzipped])

Details

name             parsed_size           gzip_size
signup                +193 B  (+0.0%)      +43 B  (+0.0%)
jetpack-connect       +193 B  (+0.0%)      +36 B  (+0.0%)
accept-invite         +193 B  (+0.0%)      +43 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.

Async-loaded Components (~3899 bytes added 📈 [gzipped])

Details

name                                           parsed_size            gzip_size
async-load-calypso-blocks-editor-launch-modal     +21117 B  (+12.2%)    +3899 B  (+8.6%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

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.

razvanpapadopol · 2020-11-18T11:31:50Z

By looking at how this prop is used, we probably want keys to be typed as plan slugs.

tjcafferkey · 2020-11-18T11:12:37Z

Non-blocker: Not a big deal just me being picky, but we could move this into the above selector to avoid repeating the selector.

.go-back-button__focused-launch { //... &:hover { opacity: 0.7; } }

tjcafferkey · 2020-11-18T11:27:09Z

If domain is null or undefined you will get these values assigned to hasPaidDomain and it'll be coerced into a falsely value.

If you changed const hasPaidDomain = domain && ! domain.is_free; to const hasPaidDomain = domain?.is_free === false; then you'll get a more predictable boolean returned.

Not a blocker though, as it'll be coerced into a falsely value anyway.

Great idea but Typescript won't like because is_free is typed wrong.

tjcafferkey · 2020-11-18T11:31:31Z

We should export these values from the package similar to how domain picker has done here so we can use it like this

tjcafferkey · 2020-11-18T11:31:36Z

We should export these values from the package similar to how domain picker has done here so we can use it like this

tjcafferkey · 2020-11-18T11:32:02Z

We should export these values from the package similar to how domain picker has done here so we can use it like this

tjcafferkey · 2020-11-18T11:32:29Z

We should export these values from the package similar to how domain picker has done here so we can use it like this

tjcafferkey · 2020-11-18T11:35:01Z

If we export the CTAVariation values, we can use them here instead of hard coding 'NORMAL'

I think exporting the type is enough. The IDE won't let you set an invalid value.

tjcafferkey · 2020-11-18T11:35:34Z

If we export the popularBadgeVariation values, we can use them here instead of hard coding 'ON_TOP'

Same as above.

tjcafferkey · 2020-11-18T11:35:58Z

If we export the popularBadgeVariation values, we can use them here instead of hard coding 'NEXT_TO_NAME'

Same as above.

tjcafferkey · 2020-11-18T11:36:04Z

If we export the popularBadgeVariation values, we can use them here instead of hard coding 'NEXT_TO_NAME'

Same as above.

razvanpapadopol · 2020-11-18T11:40:01Z

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.

tjcafferkey · 2020-11-18T11:40:50Z

Should we be able to "Deselect" a plan?

razvanpapadopol · 2020-11-18T11:53:17Z

Should we be able to "Deselect" a plan?

No, once we click Select on a plan, the modal is closed and we land back on the Summary View with that plan highlighted.
The behaviour should be similar to the one for DomainsDetails View described in #46866 (comment)

tjcafferkey · 2020-11-18T12:05:19Z

Can we use Back Button from @automattic/onboarding? Seen here

An idea would be that GoBackButton should use BackButton and then reuse here and in DomainDetails View. See also #47497 (comment)

Great idea. Didn't know about this one.

ciampo · 2020-11-18T12:27:24Z

Out of curiosity, what are the benefits of import * as React over import React ?

I thought this. Looks like it's being used for the functional component typings but not sure the benefit of *

I thought it would work anyway

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 🙏

If you do import React... you need allowSyntheticDefaultImports = 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 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#18102

[import * as React from "react";] is the correct way to import React from an ES module since the ES module will not have a default export. Only named exports.

I'm doing some changes to named exports and this will stop working. For open source ES modules we'll probably publish some default object that warns as an upgrade path but we can't use that in our own code.

We could add a lint rule for this and/or codemod it, but I don't know whether there is significant impact beyond consistency.

Thank you for the explanation! I learnt something new

If you do import React... you need allowSyntheticDefaultImports = true in tsconfig. import * as React is more semantically accurate and makes TypeScript happy.

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).

alshakero · 2020-11-18T13:05:16Z

Addressed all. Thanks everyone!

tjcafferkey · 2020-11-18T15:55:18Z

If you look at the linked issue which contains the designs and compare to the below screenshot the back button is missing some CSS lovin'

Font color
Margin bottom
Left chevron

@ciampo are we sticking with the standard <BackButton /> styles or overwriting some so it's inline with the designs?

ciampo · 2020-11-18T16:09:20Z

If you look at the linked issue which contains the designs and compare to the below screenshot the back button is missing some CSS lovin'

Font color

Margin bottom

Left chevron

@ciampo are we sticking with the standard <BackButton /> styles or overwriting some so it's inline with the designs?

Good finds!

I'd say for sure that margin should be addressed because it doesn't really have to do with the component itself
For the left chevon, we should probably add it to the <BackButton /> and enable/disable it with a prop? Something like <BackButton showChevron={true} />
Regarding font color, I'm almost tempted to leave it as it is, as the overall color palette should ultimately come from the onboarding package

What do you think?

tjcafferkey · 2020-11-18T16:13:45Z

@ciampo I think we can address the chevron issue by including the <Icon /> component as a child like so

<BackButton onClick={ goBack }>
	<Icon icon={ chevronLeft } />
	<span>{ __( 'Go back', __i18n_text_domain__ ) }</span>
</BackButton>

As for the margin, we should address this with a shared CSS class so it's consistent and leaving the color as it is I think makes sense as you said.

tjcafferkey · 2020-11-18T16:24:34Z

@ciampo are we sticking with the standard styles or overwriting some so it's inline with the designs?

@alshakero we reached a conclusion on this here 27382-pb

tjcafferkey

I can't approve as it's my original PR but this LGTM!

wp-desktop

WordPress Desktop CI Failure for job "wp-desktop-mac".

@alshakero please inspect this job's build steps for breaking changes at this link. For temporal failures, you may try to "Rerun Workflow from Failed".

Please also ensure this branch is rebased off latest Calypso.

tjcafferkey added this to the Focused Launch: MVP milestone Nov 12, 2020

tjcafferkey added [Status] In Progress Feature [Launch] Launching a site from Coming Soon to Published. labels Nov 12, 2020

tjcafferkey assigned alshakero Nov 12, 2020

alshakero force-pushed the update/focused-launch-all-plans-view branch from 74c1949 to f80a70a Compare November 16, 2020 13:53

alshakero added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Nov 16, 2020

alshakero marked this pull request as ready for review November 16, 2020 13:53

tjcafferkey requested a review from a team as a code owner November 16, 2020 18:04

alshakero force-pushed the update/focused-launch-all-plans-view branch from d8d7248 to fc0a010 Compare November 16, 2020 18:05

alshakero removed the request for review from a team November 16, 2020 18:05

razvanpapadopol reviewed Nov 18, 2020

View reviewed changes

tjcafferkey commented Nov 18, 2020

View reviewed changes

razvanpapadopol reviewed Nov 18, 2020

View reviewed changes

tjcafferkey commented Nov 18, 2020

View reviewed changes

ciampo reviewed Nov 18, 2020

View reviewed changes

tjcafferkey and others added 8 commits November 18, 2020 15:33

Plan details view initial commit

c2f6212

add 'plans-grid' reference to tsconfig

bc5b97e

Enhance plans full page

db82c09

Implement detailed plans page

736b6ce

Add locale parameter to PlansGrid

565027d

Keep track of user's paid plan choice

e067ec0

Handle domain picking button in plans grid (rather poorly)

dd18974

Address feedback

02997af

alshakero force-pushed the update/focused-launch-all-plans-view branch from 196aac6 to 02997af Compare November 18, 2020 14:34

Re-implement back button

00bf30e

tjcafferkey commented Nov 18, 2020

View reviewed changes

Fix types

ce885aa

alshakero merged commit 254f872 into master Nov 18, 2020

alshakero deleted the update/focused-launch-all-plans-view branch November 18, 2020 18:25

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 18, 2020

wp-desktop requested changes Nov 18, 2020

View reviewed changes

Conversation

tjcafferkey commented Nov 12, 2020 • edited by alshakero Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request

Testing instructions

Uh oh!

matticbot commented Nov 12, 2020

Uh oh!

matticbot commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alshakero Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alshakero Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjcafferkey commented Nov 18, 2020

Uh oh!

razvanpapadopol commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

razvanpapadopol Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjcafferkey Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tjcafferkey commented Nov 12, 2020 •

edited by alshakero

Loading

matticbot commented Nov 16, 2020 •

edited

Loading

alshakero Nov 18, 2020 •

edited

Loading

alshakero Nov 18, 2020 •

edited

Loading

razvanpapadopol commented Nov 18, 2020 •

edited

Loading

razvanpapadopol Nov 18, 2020 •

edited

Loading

tjcafferkey Nov 18, 2020 •

edited

Loading