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

Add WooCommerce onboarding styles to the WC Helper connection flow #35193

Merged
merged 4 commits into from
Aug 21, 2019

Conversation

justinshreve
Copy link
Contributor

@justinshreve justinshreve commented Aug 7, 2019

Closes woocommerce/woocommerce-admin#2600. This PR implements the Muriel WooCommerce onboarding designs to the helper OAuth login and sign-up flows. This uses a special query parameter, so other connection flows will remain the same for now.

Related (where this flow was implemented for the Jetpack connection): #33073, #34380.

Screenshots

Screen Shot 2019-08-07 at 11 06 42 AM

Screen Shot 2019-08-07 at 11 06 54 AM

Screen Shot 2019-08-07 at 11 07 09 AM

Testing Directions

  • Run master of https://github.com/woocommerce/wc-admin.
  • In your local WordPress install, add define( 'WOOCOMMERCE_CALYPSO_LOCAL', true ); to your wp-config.php.
  • Logout of WooCommerce.com and WordPress.com (otherwise you may immediately end up on the authorization screen).
  • Go to WooCommerce > Extensions > WooCommerce.com Subscriptions and disconnect from WooCommerce.com.
  • Go to the dashboard and click the connection task.
  • If you have the above patches applied, you should end up on the onboarding themed login page. Verify things look correct, you can login, and that you can also go to the signup form.
  • Verify things look correct on the signup form, you can signup, etc.
  • You can change wccom-from=onboarding in the header to wccom-from=cart and view the cart stepper header.
  • Logging in or signing up should redirect you to the unthemed WooCommerce.com connection page.

Additional testing:

In wp-calypso edit the config/development.json feature flag file and disable the new code by setting woocommerce/onboarding-oauth to false. Restart Calypso. You can test the connection again and verify that you see the existing OAuth styles.

Additional Notes/Questions

  • During a rebase, the Apple sign-in button was added. This area is starting to feel a bit like Nascar with multiple sign-in buttons that take up the same space as the normal login field. I left it alone for now, but was wondering if we want to do something in our flow @jameskoster? We can address this in a follow-up PR.
  • The lost password flow still has WP.com colors. I will create a separate issue for this as it will require more work to figure out how to get this to work per the designs.

@justinshreve justinshreve added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 7, 2019
@justinshreve justinshreve requested review from a team as code owners August 7, 2019 15:39
@justinshreve justinshreve self-assigned this Aug 7, 2019
@matticbot
Copy link
Contributor

</div>
<div className="woocommerce-connect-cart-header__stepper-step-text">
<span className="woocommerce-connect-cart-header__stepper-step-label">
{ translate( 'Installation' ) }
Copy link

Choose a reason for hiding this comment

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

Hi! I've found a possible matching string that has already been translated 38 times:
translate( 'Installation', { context: 'Navigation item'} ) ES Score: 13

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

@matticbot
Copy link
Contributor

matticbot commented Aug 7, 2019

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

App Entrypoints (~182 bytes removed 📉 [gzipped])

name                   parsed_size           gzip_size
entry-main                  -971 B  (-0.1%)      -41 B  (-0.0%)
entry-domains-landing       -477 B  (-0.1%)     -141 B  (-0.1%)

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

Sections (~740 bytes removed 📉 [gzipped])

name                  parsed_size           gzip_size
login                     +4214 B  (+3.0%)     +911 B  (+2.4%)
import                     -553 B  (-0.3%)     -131 B  (-0.2%)
themes                     -552 B  (-0.1%)     -148 B  (-0.1%)
theme                      -552 B  (-0.2%)     -148 B  (-0.2%)
woocommerce                -551 B  (-0.0%)       -6 B  (-0.0%)
stats                      -551 B  (-0.1%)     -163 B  (-0.1%)
settings-writing           -551 B  (-0.1%)       -9 B  (-0.0%)
settings-security          -551 B  (-0.2%)       -9 B  (-0.0%)
settings-performance       -551 B  (-0.3%)       -9 B  (-0.0%)
settings-discussion        -551 B  (-0.3%)       -9 B  (-0.0%)
settings                   -551 B  (-0.1%)       -9 B  (-0.0%)
post-editor                -551 B  (-0.0%)      -24 B  (-0.0%)
marketing                  -551 B  (-0.1%)       -9 B  (-0.0%)
comments                   -551 B  (-0.1%)     -184 B  (-0.1%)
checkout                   -551 B  (-0.1%)     -170 B  (-0.1%)
activity                   -551 B  (-0.1%)     -163 B  (-0.1%)
gutenberg-editor           -478 B  (-0.1%)       -2 B  (-0.0%)
plans                      -476 B  (-0.1%)     -171 B  (-0.1%)
posts-pages                -475 B  (-0.1%)      -43 B  (-0.1%)
signup                     -466 B  (-0.2%)     -161 B  (-0.2%)
jetpack-connect            -287 B  (-0.0%)     -127 B  (-0.1%)
accept-invite              +189 B  (+0.1%)      +44 B  (+0.1%)

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 (~897 bytes added 📈 [gzipped])

name                                                         parsed_size           gzip_size
async-load-signup-steps-user                                     +4415 B  (+4.9%)     +939 B  (+4.0%)
async-load-design-blocks                                         +3065 B  (+0.1%)     +614 B  (+0.1%)
async-load-my-sites-checklist-wpcom-checklist-component-jsx      -1029 B  (-0.7%)      +53 B  (+0.2%)
async-load-signup-steps-import-url-onboarding                     -553 B  (-1.4%)      -10 B  (-0.1%)
async-load-signup-steps-import-url                                -553 B  (-2.1%)      -10 B  (-0.1%)
async-load-signup-steps-clone-point                               -551 B  (-0.3%)     -163 B  (-0.4%)
async-load-blocks-calendar-popover                                -551 B  (-0.2%)     -184 B  (-0.4%)
async-load-signup-steps-plans-atomic-store                        -476 B  (-0.4%)     -171 B  (-0.6%)
async-load-signup-steps-plans                                     -476 B  (-0.3%)     -171 B  (-0.4%)

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.

@justinshreve justinshreve force-pushed the add/wccom-connect-onboarding-styles branch 2 times, most recently from 2ec5b8e to 1e11edf Compare August 7, 2019 17:42
Copy link
Contributor

@jeffstieler jeffstieler left a comment

Choose a reason for hiding this comment

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

This looks good and tests well with the provided instructions.

I'd feel most comfortable with another review though, since it's been quite a while since I've danced to Calypso 😛

Copy link
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

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

Code looks good as far as I can tell.

@justinshreve justinshreve force-pushed the add/wccom-connect-onboarding-styles branch from 1e11edf to a6a21fe Compare August 20, 2019 16:45
…ibing the client IDs, and adjust social button CSS
@justinshreve justinshreve added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 20, 2019
@justinshreve justinshreve merged commit 968d71a into master Aug 21, 2019
@justinshreve justinshreve deleted the add/wccom-connect-onboarding-styles branch August 21, 2019 14:04
@jsnajdr
Copy link
Member

jsnajdr commented Sep 27, 2019

Hi @justinshreve 👋 I found this PR when testing some login CSS changes, and I don't understand some of the style changes what were made here.

There are CSS rules added to client/signup/style.scss, a stylesheet that loads only in Signup (i.e., when inside the /start routes), but they style login:

.layout.is-wccom-oauth-flow {
  .login-form__footer { ... }
  .wp-login__footer-links { ... }
  .login__woocommerce-logo { ... }
}

It's not at all guaranteed that Signup styles are present when showing a Login page and the styling is going to break randomly.

Could you shed some light on what UI and flows are supposed to be restyled here? It's difficult to figure out the various Woo + Jetpack + OAuth flows and test them.

@justinshreve
Copy link
Contributor Author

Hi @jsnajdr - Thanks for the ping.

We have a custom header for these Woo OAuth flows (see in the screenshots above, either the cart display, Woo + JP logos, or just the Woo Logo): https://github.com/Automattic/wp-calypso/pull/35193/files#diff-066c9eda2e0b68f9cab2487000375fb6R250-R274

It looks like I had named the classes login-form__footer, etc hence the rules in signup to style them -- so they are present on those routes. But I agree these should use signup named classes. I will clean that up.

Could you shed some light on what UI and flows are supposed to be restyled here?
It's difficult to figure out the various Woo + Jetpack + OAuth flows and test them.

This PR addresses the flow WooCommerce core uses to connect to WooCommerce.com (so the WooCommerce Helper can do things like extension updates). This is a styled OAuth2 flow like Crowdsignal.

There is also a Woo and Jetpack connection flow (to make things like WooCommerce Services, label printing, etc work). which also has a stylized sign-up and login (#32993).

Please see 22a33-pb for links to designs of these two flows. This will give you an idea of how they work.

Just a note that neither of these two flows are launched yet and are a part of a larger overhaul to WooCommerce core's onboarding experience. They are a bit difficult to test since you have to enter the flows from the right paths, which live inside a development build of WooCommerce Admin.

We are working on a test build of the plugin that will make this easier, but in the meantime the best way to load these is the following:

  • npm start on wp-calypso master.
  • Install WooCommerce
  • Install WooCommerce Admin from GitHub (https://github.com/woocommerce/woocommerce-admin) and npm run dev.
  • define( 'WOOCOMMERCE_CALYPSO_ENVIRONMENT', 'development' ); in your wp-config.php.
  • define( 'WP_DEBUG', true ); in your wp-config.php.
  • Go to /wp-admin/edit.php?post_type=shop_order, click the Help Tab.
  • Under Setup Wizard you should see a Calypso / WordPress.com section for quickly testing the Jetpack flow, or testing the OAuth flow here.

@scinos
Copy link
Contributor

scinos commented Jun 2, 2020

I found this PR when writing tests for the server and I think it may be broken. In particular, looks like this check will always be false:

const isWCComConnect =
	config.isEnabled( 'woocommerce/onboarding-oauth' ) &&
	( 'login' === request.context.sectionName || 'signup' === request.context.sectionName ) &&
	request.query[ 'wccom-from' ] &&
	isWooOAuth2Client( { id: parseInt( oauthClientId ) } );	

The reason is that when getDefaultContext runs, request.context.sectionName is undefined.

When the PR was made, the original code to set sectionName was (edited)

app.get( pathRegex, function( req, res, next ) {
	req.context = Object.assign( {}, req.context, { sectionName: section.name } );
	//...
	next();
} );

if ( ! section.isomorphic ) {
	app.get( pathRegex, setUpRoute, serverRender );
}

When a request comes in, it first run the "inline" middleware that sets sectionName, then it runs setUprRoute middleware that ends up callinggetDefaultContext(). At that point sectionName is already set and everything works as expected.

However, right now in master the equivalent code is:

app.get( pathRegex, setupDefaultContext( entrypoint ), function ( req, res, next ) {
	req.context.sectionName = section.name;
	//..
} );

Now when a request comes in, it first calls setupDefaultContext() (which calls getDefaultContext()) and then sectionName is set, resulting in sectionName not being defined and therefore isWCComConnect always set as false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Onboarding Design on WCCOM Account/Login Pages
7 participants