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

Woo landing page design iterations #60697

Merged
merged 14 commits into from Feb 2, 2022
Merged

Woo landing page design iterations #60697

merged 14 commits into from Feb 2, 2022

Conversation

lsl
Copy link
Contributor

@lsl lsl commented Feb 1, 2022

Changes proposed in this Pull Request

Testing instructions

Before

Screenshot 2022-02-01 at 17-42-41 WooCommerce — WordPress com

After
Screenshot 2022-02-01 at 17-41-21 WooCommerce — WordPress com

Related to #59190

@lsl lsl requested a review from a team February 1, 2022 04:57
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 1, 2022
@matticbot
Copy link
Contributor

matticbot commented Feb 1, 2022

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

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

name         parsed_size           gzip_size
entry-main         +60 B  (+0.0%)      +16 B  (+0.0%)
entry-login        +60 B  (+0.0%)      +16 B  (+0.0%)

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

Sections (~1877 bytes added 📈 [gzipped])

name                      parsed_size           gzip_size
woocommerce-installation      +3609 B  (+1.2%)    +1877 B  (+2.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.

@lsl
Copy link
Contributor Author

lsl commented Feb 1, 2022

Tofix (blocking merge):

  • Add disabled button support to EmptyContent component, disable the cta when transferring is blocked. Existing code did this, new code in 60697 does not.
  • Hide the extra CTA (again)
  • Some css lints need fixing (--no-verified it as it was copy pasted from styled divs in CtaSection)

Todo:

  • Replace illustration with new one (The shopping bags one is fine for now)
  • Spacing / whitespace / margins need fixing up
  • Delete removed images (I don't think anything else uses them)
  • Update colophon link to plugins/woocommerce/


.woocommerce_landing-page {
.empty-content__title {
font-family: Recoleta;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels like something that should be done in the EmptyContent component directly but would widen testing considerably. (see "Inbox" page at the moment for why I think it might be worth doing)

Copy link
Member

Choose a reason for hiding this comment

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

It looks like it's used pretty widely. 😬 I think we're okay keeping it here for now, but I see what you mean about the Inbox page, where it's missing that font.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am with @allilevine in this one this will widen testing a lot, I also feel we can totally add some focused personalization tho the base component through css per page like we are doing right now

@lsl lsl self-assigned this Feb 1, 2022
</CtaSection>
{ renderWarningNotice() }
<EmptyContent
title={ __( 'Set up a store and start selling online' ) }
Copy link
Member

Choose a reason for hiding this comment

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

I changed this to "Set up" rather than "Setup" because we say "Set up a new store..." in the subheader. And setup is a noun. 🙂

@allilevine allilevine requested a review from a team February 1, 2022 20:24
Copy link
Member

@obenland obenland left a comment

Choose a reason for hiding this comment

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

This is close for a first pass, I just had a couple of questions.

import Image01 from 'calypso/assets/images/woocommerce/woop-cta-image01.jpeg';
import Image02 from 'calypso/assets/images/woocommerce/woop-cta-image02.jpeg';
import Image03 from 'calypso/assets/images/woocommerce/woop-cta-image03.jpeg';
import Image04 from 'calypso/assets/images/woocommerce/woop-cta-image04.jpeg';
Copy link
Member

Choose a reason for hiding this comment

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

Should we delete those images since they're not being used anymore?

Copy link
Member

Choose a reason for hiding this comment

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

That's on the list as a non-blocking change I believe: #60697 (comment) Do you think it's a blocker?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha! In that case, no :)

| `actionHoverCallback`\* | `bool` | `0` | Indicates activity while a background action is being performed. |
| `actionHoverCallback`\* | `bool` | `0` | Indicates activity while a background action is being performed. |
| `actionDisabled` | `bool` | `null` | Disables the button. |
| `actionRef` | `function` | `null` | Adds a ref to the button. |
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the linter picked up some whitespace issues in this file?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I missed that!

Copy link
Contributor

@Rolilink Rolilink left a comment

Choose a reason for hiding this comment

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

There are some minor design changes like:

  • Making tittle and subtitle container smaller to match design
  • Switch buttons positions

But this seems launchable!

@obenland obenland 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 Feb 1, 2022
@matticbot
Copy link
Contributor

This PR modifies the release build for happychat

To test your changes on WordPress.com, run install-plugin.sh happychat update/woop-landing on your sandbox.

To deploy your changes after merging, see the documentation: TODO

@lsl lsl merged commit 3f8fead into trunk Feb 2, 2022
@lsl lsl deleted the update/woop-landing branch February 2, 2022 01:03
@a8ci18n
Copy link

a8ci18n commented Feb 2, 2022

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7123735

Thank you @lsl for including a screenshot in the description! This is really helpful for our translators.

@a8ci18n
Copy link

a8ci18n commented Feb 11, 2022

Translation for this Pull Request has now been finished.

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

Successfully merging this pull request may close these issues.

None yet

6 participants