-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Jetpack Search: adapt post-checkout modals #40481
Conversation
8dc565b
to
0805513
Compare
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~254 bytes added 📈 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
0805513
to
a487576
Compare
@AnnaMag @gibrown Following up our quick call, let me know if the changes below make sense:
I'll also add these as suggestions on the code. |
client/my-sites/plans/current-plan/current-plan-thank-you/search-thank-you.js
Outdated
Show resolved
Hide resolved
client/my-sites/plans/current-plan/current-plan-thank-you/search-thank-you.js
Outdated
Show resolved
Hide resolved
client/my-sites/plans/current-plan/current-plan-thank-you/thank-you.js
Outdated
Show resolved
Hide resolved
client/my-sites/plans/current-plan/current-plan-thank-you/thank-you.js
Outdated
Show resolved
Hide resolved
Question -- how do they know (or can they) when indexing is done? I'd be nice to tell them that here, if there's a notification or a screen they can check or something. |
@keoshi would you rather use the illustration from Automattic/jetpack#15074? |
@AnnaMag Nah, let's go with what we have here to highlight the moment of celebration. The other illustration is a bit bland, even if more relevant to the product. |
@michelleweber Glad you noticed that! :) We did propose to have a different copy during the design phases:
But it seems that this bit won't be ready for the launch phase. Pinging @gibrown for confirmation though. Thank you again, Michelle! |
That copy sounds good and ya we can't currently tell them when indexing is complete (or rather we aren't tying that into this project). |
<Button | ||
primary | ||
compact={ true } | ||
href="customize.php?autofocus[section]=jetpack_search" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick note here that this link doesn't work yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is in the making
I gave this a quick test and it worked great other than the links. |
6862c6d
to
77934cd
Compare
77934cd
to
434c2ef
Compare
How can I do this step? |
client/my-sites/plans/current-plan/current-plan-thank-you/search-thank-you.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a minor fix to have the customize link go to the jpsearch panel. Looks good and works well.
YES! Thank you for that change, @gibrown ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't test it, but the screenshot looks good. I just added a few suggestions based on a test I did with the existing thank you modal.
Any reason why we have to use is-compact
buttons here? If not, then please see if my changes make sense.
client/my-sites/plans/current-plan/current-plan-thank-you/thank-you.js
Outdated
Show resolved
Hide resolved
client/my-sites/plans/current-plan/current-plan-thank-you/thank-you.js
Outdated
Show resolved
Hide resolved
client/my-sites/plans/current-plan/current-plan-thank-you/style.scss
Outdated
Show resolved
Hide resolved
client/my-sites/plans/current-plan/current-plan-thank-you/style.scss
Outdated
Show resolved
Hide resolved
1b0d920
to
1be9077
Compare
@keoshi good catch. Apologies for unclear test instructions! I pushed the changes and retested. Looks great, thank you. |
No problem. Thank you! 🛳 |
@gibrown Test failures have been fixed. |
Search: add CTA buttons to the thank you modal and set the primary button
c4d958e
to
6c4f5e6
Compare
Changes proposed in this Pull Request
Testing instructions
/plans
for a JP connected site on a non-Professional planWP-Admin
Fixes #40434
Sequel: add tracking