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.

Sign up for GitHub

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

Jump to bottom

Jetpack Connect: adapt the Search card copy on the plans page in the absence of site info. #41276

Merged
merged 5 commits into from
Apr 29, 2020
Merged

Jetpack Connect: adapt the Search card copy on the plans page in the absence of site info. #41276

merged 5 commits into from
Apr 29, 2020

Conversation

AnnaMag
Copy link
Contributor

@AnnaMag AnnaMag commented Apr 20, 2020

Changes proposed in this Pull Request

  • When accessing the canonical plans page at jetpack/connect/store, we do not have site information available. Thus, we cannot estimate record size to be indexed nor price of the Search product. Here, we remove the relevant parts from the copy.

After:

image

Testing instructions

  • go to http://calypso.localhost:3000/jetpack/connect/store for the local build
  • verify that the copy for Search agrees with the above design and does not affect the remaining product and plans

Fixes #41025

@AnnaMag AnnaMag added [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Status] In Progress [Feature] Search Jetpack Search - A feature to speed up and improve the look of Search on your site. labels Apr 20, 2020
@AnnaMag AnnaMag requested review from keoshi, gibrown and jsnmoon April 20, 2020 12:25
@AnnaMag AnnaMag self-assigned this Apr 20, 2020
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/remove-search-cost-ref

@matticbot
Copy link
Contributor

matticbot commented Apr 20, 2020 •
edited
Loading

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

Sections (~188 bytes added 📈 [gzipped])

name             parsed_size           gzip_size
plans                 +398 B  (+0.1%)      +94 B  (+0.1%)
jetpack-connect       +398 B  (+0.1%)      +94 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 (~186 bytes added 📈 [gzipped])

name                           parsed_size           gzip_size
async-load-signup-steps-plans       +398 B  (+0.2%)      +94 B  (+0.2%)
async-load-design-blocks            +398 B  (+0.0%)      +92 B  (+0.0%)

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.

@AnnaMag AnnaMag force-pushed the fix/remove-search-cost-ref branch 2 times, most recently from 50a7754 to 02947af Compare April 20, 2020 22:17
@AnnaMag AnnaMag 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 Apr 23, 2020
@jsnmoon
Copy link
Contributor

jsnmoon commented Apr 24, 2020

I'll take a closer look at this on Monday, but it looks like the Prettier formatting changes might have been unintentional. I just reformatted the file with a clean yarn install and found that it reverted most of the formatting changes.

Also: this PR needs a rebase.

@AnnaMag AnnaMag force-pushed the fix/remove-search-cost-ref branch 2 times, most recently from 4d188eb to a9fb946 Compare April 25, 2020 10:30
@AnnaMag
Copy link
Contributor Author

AnnaMag commented Apr 25, 2020

Yes, it was unintentional- corrected and rebased. Ready for 👀

keoshi
keoshi approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@keoshi keoshi left a comment

Choose a reason for hiding this comment

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

Design-wise this looks great to me!

jsnmoon
jsnmoon suggested changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@jsnmoon jsnmoon left a comment

Choose a reason for hiding this comment

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

There's a blocking regression that prevents the product management button from rendering depending on the selected billing timeframe; it effectively reverts #41262.

client/blocks/product-selector/index.jsx Outdated Show resolved Hide resolved
client/blocks/product-selector/index.jsx Outdated Show resolved Hide resolved
client/blocks/product-selector/index.jsx Outdated Show resolved Hide resolved
@AnnaMag AnnaMag force-pushed the fix/remove-search-cost-ref branch 3 times, most recently from 1a97eb5 to c41a17c Compare April 28, 2020 14:51
@AnnaMag
Copy link
Contributor Author

AnnaMag commented Apr 28, 2020

Feedback addressed. Thank you.

@AnnaMag AnnaMag requested a review from jsnmoon April 28, 2020 15:11
jsnmoon
jsnmoon approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@jsnmoon jsnmoon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! Tested on /jetpack/connect/store and /plans/monthly/<site>.

Not sure why the mobile E2E's are failing, though.

@jsnmoon jsnmoon 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 Apr 28, 2020
AnnaMag added 2 commits April 29, 2020 19:17
@AnnaMag
Plans: adapt the Search card copy when no site info is present.
d286e4e 
When accessing the canonical plans page at /store, we do not have site information
available. Thus, we cannot estimate record size nor price of the Search product.
Here, we remove the relevant parts from the copy.
@AnnaMag
Plans: remove mention of savings on the Search product card.
02e5149
@AnnaMag AnnaMag force-pushed the fix/remove-search-cost-ref branch from c41a17c to c79f780 Compare April 29, 2020 17:17
@AnnaMag AnnaMag merged commit 5d16324 into master Apr 29, 2020
@AnnaMag AnnaMag deleted the fix/remove-search-cost-ref branch April 29, 2020 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavidRothstein DavidRothstein DavidRothstein left review comments

@keoshi keoshi keoshi approved these changes

@jsnmoon jsnmoon jsnmoon approved these changes

@gibrown gibrown Awaiting requested review from gibrown

Assignees

@AnnaMag AnnaMag

Labels
[Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Feature] Search Jetpack Search - A feature to speed up and improve the look of Search on your site.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Jetpack Connect: "0 records" and lowest search tier price always shown on pre-connection products page
5 participants
@AnnaMag @matticbot @jsnmoon @DavidRothstein @keoshi