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: add PlansGrid accordion #45054

Merged
merged 9 commits into from Sep 10, 2020
Merged

Conversation

razvanpapadopol
Copy link

@razvanpapadopol razvanpapadopol commented Aug 20, 2020

Changes proposed in this Pull Request

  • Re-add plans accordion in Gutenboarding and Site setup. Note: PlansGrid accordion was first introduced in Gutenboarding: Accordion-style plans grid. #44299
  • Ship Feature picker and Plans Grid accordion to Gutenboarding "stable" (still behind gutenboarding/feature-picker flag in production environment).
  • Update Gutenboarding e2e test.

Testing instructions

  • Go to /new
  • Before reaching Plans step in the linear flow, you should land on Feature selection step.
  • After skipping or confirming feature selection, Plans step should display as an accordion.
  • After creating the site, apply D48381-code and sandbox the new site.
  • When in editor Site setup flow should include PlansGrid accordion.

@razvanpapadopol razvanpapadopol added [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Goal] New Onboarding previously called Gutenboarding labels Aug 20, 2020
@razvanpapadopol razvanpapadopol self-assigned this Aug 20, 2020
@matticbot
Copy link
Contributor

matticbot commented Aug 20, 2020

@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 20, 2020
@razvanpapadopol razvanpapadopol force-pushed the add/gutenboarding-experimental-features branch from 516473c to e962d01 Compare August 20, 2020 10:37
@simison
Copy link
Member

simison commented Aug 20, 2020

This still wouldn't launch it to customers, right? Either because we'd not be directing traffic to "experimental" flow yet or otherwise?

@matticbot
Copy link
Contributor

matticbot commented Aug 20, 2020

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

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

name                 parsed_size           gzip_size
entry-gutenboarding      +6316 B  (+0.4%)    +1179 B  (+0.3%)

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

Sections (~26 bytes removed 📉 [gzipped])

name              parsed_size           gzip_size
gutenberg-editor        -49 B  (-0.0%)      -26 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.

@razvanpapadopol razvanpapadopol changed the title Gutenboarding: Re-add PlansGrid accordion version Gutenboarding: Re-add PlansGrid accordion as experimental feature Aug 20, 2020
@razvanpapadopol razvanpapadopol changed the title Gutenboarding: Re-add PlansGrid accordion as experimental feature Gutenboarding: re-add PlansGrid accordion as experimental feature Aug 20, 2020
@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D48381-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

Base automatically changed from add/gutenboarding-experimental-features to master August 20, 2020 12:49
@razvanpapadopol
Copy link
Author

razvanpapadopol commented Aug 20, 2020

This still wouldn't launch it to customers, right? Either because we'd not be directing traffic to "experimental" flow yet or otherwise?

Yes, the experimental branch containing feature picker and accordion plans grid would be hidden behind ?latest query param for both Gutenboarding site creation and Complete Setup flows.

Once this is tested and merged (together with Editing Plugin and wpcom-block-editor widget) we can start sending traffic from some A/B tests.

isOpen={ isOpen }
onPickDomain={ onPickDomainClick }
disabledLabel={ disabledLabel }
/>
Copy link
Author

Choose a reason for hiding this comment

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

This is the main change that was actually introduced in this PR. Everything else is mostly just reverting the accordion styled PlansGrid revert :bowtie: while keeping the "stable" multi-column PlansGrid.

I've decided to extract PlansFeatureList since it's almost identical in both versions of the PlansGrid. I've added multiColumn prop for now but I'm pretty sure the same effect can be achieved with a fluid layout (decide on the minimum width and let flex-wrap to automatically decide the number of columns).

cc: @yansern @alshakero

@razvanpapadopol razvanpapadopol force-pushed the add/plans-grid-accordion branch 2 times, most recently from 46264e6 to e2fda01 Compare August 24, 2020 13:40
@razvanpapadopol razvanpapadopol changed the title Gutenboarding: re-add PlansGrid accordion as experimental feature Gutenboarding: add experimental features to Site setup Aug 27, 2020
@razvanpapadopol
Copy link
Author

Superseded by #45280

@razvanpapadopol razvanpapadopol changed the title Gutenboarding: add PlansGrid accordion as experimental feature Gutenboarding: add PlansGrid accordion Sep 9, 2020
@razvanpapadopol razvanpapadopol marked this pull request as ready for review September 9, 2020 18:05
@simison
Copy link
Member

simison commented Sep 10, 2020

@razvanpapadopol there's at least one E2E failure from Gutenboarding that needs fixing.

@@ -351,7 +351,7 @@ class CalypsoifyIframe extends Component<
this.props.siteCreationFlow === 'gutenboarding' && this.props.isSiteUnlaunched;
const frankenflowUrl = `${ window.location.origin }/start/new-launch?siteSlug=${ this.props.siteSlug }&source=editor`;
const isNewLaunch = config.isEnabled( 'gutenboarding/new-launch' );
const isExperimental = new URLSearchParams( window?.location.search ).has( 'latest' );
const isExperimental = config.isEnabled( 'gutenboarding/feature-picker' );
Copy link
Member

@simison simison Sep 10, 2020

Choose a reason for hiding this comment

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

This "isExperimental" = "feature-picker" terminology can feel a bit confusing later on but I think it's fine now, and we'll sort it out later.

Copy link
Author

Choose a reason for hiding this comment

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

Right now this would be easiest stable way to flip the switch between multi-column PlansGrid and accordion PlansGrid + Feature picker for both Gutenboarding and Site Setup flows.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense 👍

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

I'll test little more but two small visual findings. These can be follow-up PRs too:

Price loading state is a bit funky:
Screenshot 2020-09-10 at 11 36 16

Hiding the /mo completely vs placing it next to loading animation probably works best and is easiest.

Another is related to scroll positions:

  • Expand all plans
  • Click on domain to move to domains step
  • Click continue
  • Now you land in plans page where everything is collapsed again and scroll position isn't on top:

Screenshot 2020-09-10 at 11 37 45

Screenshot 2020-09-10 at 11 39 09

Screenshot 2020-09-10 at 11 39 13

  • Easiest would be at least to ensure we're on top when landing on plans page
  • It might be helpful to store expanded/collapsed state during the session

Again, neither should block merging.

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

In the launch flow, with flag enabled for a site that had free plan, I landed directly at plan step:

image

I expected to land on domain step first like I do in master.

Without the flag, I see older "launch" button instead of "complete setup" button. I expected complete setup with multi-grid:

image

@razvanpapadopol
Copy link
Author

Price loading state is a bit funky:

Fixed in a659c2e
Demo on mobile: https://cloudup.com/c4NHHotDhJ7
Demo on desktop: https://cloudup.com/cOd1LafAdQG

Another is related to scroll positions:

  • Easiest would be at least to ensure we're on top when landing on plans page

Fixed in 88877d0

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Double tested with new site with Chrome; looks like the landing on site title step was caused by something else than this PR (happens also in master).

Other than that, the flow is looking good and works great. 👍 Let's get this in.

@razvanpapadopol razvanpapadopol merged commit 8a70073 into master Sep 10, 2020
@razvanpapadopol razvanpapadopol deleted the add/plans-grid-accordion branch September 10, 2020 11:36
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 10, 2020
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.

None yet

3 participants