Jetpack Search: instant search toggle should change with Jetpack Search toggle #52631

kangzj · 2021-05-06T01:42:34Z

Fixes: #52404

Changes proposed in this Pull Request

When disabling Jetpack Search, instant search would also be disabled which is a child option under Jetpack Search, which also aligns with Jetpack plugin settings page.
And, because we recommend instant search, when Jetpack is re-enabled, instant search would be re-enabled too.
Changed label to be clearer - 'Enable Jetpack Search'
Separate the setting saving of jetpack sites (including Atomic) from simple sites to avoid a random error discussed in P2.

Discussion

pcNPJE-8a-p2

Testing instructions

Open Settings->General->Performance of a site with Jetpack Search plan (Atomic/Simple/Jetpack sites) in Clypso
Ensure when Jetpack Search is toggled disabled, instant search is disabled as well.
Ensure when Jetpack Search is toggled enabled, instant search is set to enabled.
Ensure Instant search toggle could change and save without affecting Jetpack Search toggle.
Ensure it works for Atomic / Simple / Jetpack site.

Gif: https://d.pr/i/dl2HRh

github-actions · 2021-05-06T01:42:50Z

Test live: https://calypso.live/?branch=fix/instant-search-toggle

matticbot · 2021-05-06T01:43:25Z

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

Sections (~317 bytes added 📈 [gzipped])

name                  parsed_size           gzip_size
settings-performance      +1342 B  (+0.3%)     +248 B  (+0.2%)
settings-writing           +506 B  (+0.1%)     +115 B  (+0.1%)
settings-security          +506 B  (+0.2%)     +117 B  (+0.1%)
settings-discussion        +506 B  (+0.2%)     +117 B  (+0.1%)
marketing                  +506 B  (+0.1%)     +116 B  (+0.1%)
settings                   +443 B  (+0.1%)      +99 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.

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.

kangzj · 2021-05-06T21:41:25Z

When updating instant search toggle (live or on this branch), it calls /rest/v1.1/jetpack-blogs/190071516/rest-api/?http_envelope=1 to sync the Jetpack setting to the site, which sometimes fails with error code 400.

The above issue mentioned will be looked at separately.

Request

{"path":"/jetpack/v4/settings/","body":"{\"lazy-images\":true,\"photon-cdn\":false,\"photon\":false,\"instant_search_enabled\":true}","json":true}
```

Response
```JSON
{"code":400,"headers":[{"name":"Content-Type","value":"application\/json"}],"body":{"error":"some_updated","message":""}}

client/my-sites/site-settings/search.jsx

bluefuton · 2021-05-11T03:34:41Z

I notice that sometimes the instant search toggle loads before the main JP Search toggle - could we do checked={ fields.jetpack_search_enabled && fields.jetpack_instant_search_enabled } for that second toggle?

bluefuton · 2021-05-11T04:02:18Z

The "If deactivated..." part of the description seems a bit redundant now we've made the main toggle label clearer ('Enable Jetpack Search'). Might also be confusing in light of the 'submit' overlay trigger we're adding in Automattic/jetpack#19795. Shall we drop it?

kangzj · 2021-05-11T04:44:36Z

The "If deactivated..." part of the description seems a bit redundant now we've made the main toggle label clearer ('Enable Jetpack Search'). Might also be confusing in light of the 'submit' overlay trigger we're adding in Automattic/jetpack#19795. Shall we drop it?

Agree. The text is a bit vague now, removed in 96a8bef.

kangzj · 2021-05-12T01:07:01Z

I notice that sometimes the instant search toggle loads before the main JP Search toggle - could we do checked={ fields.jetpack_search_enabled && fields.jetpack_instant_search_enabled } for that second toggle?

Good point. Fixed in 5b6d040

jsnmoon

Changes look good overall. Two questions:

Would it be possible to keep the Instant Search enabled even if the Search module is disabled? (e.g. When Instant Search is enabled, the Search module is also toggled on.) This is how it currently works in /wp-admin/admin.php?page=jetpack#/performance.
What do we gain by refactoring renderSettingsCard to create renderSearchTogglesForJetpackSites and renderSearchTogglesForSimpleSites? I'm seeing a bit of duplicated code (e.g. form toggles' disabled conditional) and am curious about the rationale for this change. Is it to improve code clarity?

EDIT: Meant to say "keep the Instant Search toggle enabled" for point 1.

kangzj · 2021-05-13T00:22:23Z

Thanks for your review @jsnmoon :-)

Would it be possible to keep the Instant Search enabled even if the Search module is disabled? (e.g. When Instant Search is enabled, the Search module is also toggled on.) This is how it currently works in /wp-admin/admin.php?page=jetpack#/performance.

Hmmm, are you sure? As I tested, the way proposed in the PR works the same as it is in WP Admin - the Instant Search option toggles with the Search option. And actually this is one of the reasons to change in Calypso - to align experience.

What do we gain by refactoring renderSettingsCard to create renderSearchTogglesForJetpackSites and renderSearchTogglesForSimpleSites? I'm seeing a bit of duplicated code (e.g. form toggles' disabled conditional) and am curious about the rationale for this change. Is it to improve code clarity?

Yes it is, otherwise we'll have to test whether the site is Jetpack or not for every component (and attributes), which is not very pleasant to read. By refactoring it like above, there is a bit of duplication, but much better readability.

Co-authored-by: Chris R <chris.rosser@automattic.com>

client/my-sites/site-settings/search.jsx

bluefuton · 2021-05-13T03:49:39Z

client/my-sites/site-settings/wrap-settings-form.jsx

+						this.props.saveInstantSearchRequest?.lastUpdated ) &&
+				this.props.siteId === prevProps.siteId
+			) {
+				// NOTE: 1. the condition is pretty messy - the problem is that, if a request is the same


I trust your research here :) Works as intended!

bluefuton

Looks good! 🚢

kangzj · 2021-05-13T03:58:15Z

Looks good! 🚢

Thanks @bluefuton

So excited to get this done!!

a8ci18n · 2021-05-13T04:16:13Z

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

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

jsnmoon · 2021-05-13T19:37:58Z

Would it be possible to keep the Instant Search enabled even if the Search module is disabled? (e.g. When Instant Search is enabled, the Search module is also toggled on.) This is how it currently works in /wp-admin/admin.php?page=jetpack#/performance.

Hmmm, are you sure? As I tested, the way proposed in the PR works the same as it is in WP Admin - the Instant Search option toggles with the Search option. And actually this is one of the reasons to change in Calypso - to align experience.

Oops, I fat-fingered my initial review. I meant to say if it would be possible to keep the Instant Search toggle enabled even if the search module is disabled. This is how it currently works in wp-admin:

Video.MP4.1070x336.mp4

Since the Instant Search toggle is disabled when the module is disabled, this UX is not currently possible:

kangzj · 2021-05-13T22:35:41Z

I meant to say if it would be possible to keep the Instant Search toggle enabled even if the search module is disabled.

Ha got it. It makes sense. Thanks for reporting this, will get it addressed with #52736

cavalierlife · 2021-05-18T16:04:30Z

@kangzj I think the new wording looks good. Approved from our end.

kangzj · 2021-05-18T22:41:44Z

Thanks @cavalierlife 👍

a8ci18n · 2021-05-21T10:32:49Z

Translation for this Pull Request has now been finished.

kangzj added [Feature] Site Settings All other general site settings. [Status] In Progress [Feature] Search Jetpack Search - A feature to speed up and improve the look of Search on your site. labels May 6, 2021

kangzj self-assigned this May 6, 2021

kangzj changed the title ~~Jetpack Search: instant search toggle should change when Jetapck Search toggle changes~~ Jetpack Search: instant search toggle should change with Jetapck Search toggle May 6, 2021

bluefuton changed the title ~~Jetpack Search: instant search toggle should change with Jetapck Search toggle~~ Jetpack Search: instant search toggle should change with Jetpack Search toggle May 6, 2021

bluefuton reviewed May 6, 2021

View reviewed changes

client/my-sites/site-settings/search.jsx Outdated Show resolved Hide resolved

kangzj 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 May 11, 2021

kangzj requested a review from bluefuton May 11, 2021 02:05

bluefuton requested a review from jsnmoon May 11, 2021 02:10

bluefuton reviewed May 11, 2021

View reviewed changes

client/my-sites/site-settings/search.jsx Outdated Show resolved Hide resolved

bluefuton reviewed May 11, 2021

View reviewed changes

client/my-sites/site-settings/search.jsx Outdated Show resolved Hide resolved

bluefuton mentioned this pull request May 11, 2021

Search: instant search toggle is not clear on Business plan #52736

Closed

jsnmoon reviewed May 13, 2021

View reviewed changes

kangzj force-pushed the fix/instant-search-toggle branch from e156589 to 4b5c06c Compare May 13, 2021 00:17

kangzj added 6 commits May 13, 2021 12:25

toggle instant search option with jetpack search

fb14687

changed jetpack search label to be clearer

7fafec5

instant toggle tandem with jetpack toggle

706d16c

added comments

505a0af

rename symbol to camelcase

a01b6a7

added onChange propTypes to JetpackModuleToggle and track

e5f3eed

kangzj and others added 10 commits May 13, 2021 12:25

added trackEvent

28d62a2

call onChange only if it exists

9538064

separate jetpack site setting from simple sites

510864a

added missing propTypes

555e301

removed unused code

73e13cb

reverse the logic for better understanding

64c6171

Update client/my-sites/site-settings/search.jsx

ae75bd5

Co-authored-by: Chris R <chris.rosser@automattic.com>

Update client/my-sites/site-settings/search.jsx

d42a590

Co-authored-by: Chris R <chris.rosser@automattic.com>

tweaked instant search description

dce9bba

separate jetpack sites toggles from sipmple site for clarity

28626a4

kangzj force-pushed the fix/instant-search-toggle branch from 4b5c06c to 28626a4 Compare May 13, 2021 00:25

kangzj requested a review from bluefuton May 13, 2021 01:10

bluefuton reviewed May 13, 2021

View reviewed changes

client/my-sites/site-settings/search.jsx Show resolved Hide resolved

bluefuton reviewed May 13, 2021

View reviewed changes

bluefuton approved these changes May 13, 2021

View reviewed changes

bluefuton 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 May 13, 2021

kangzj merged commit 85d0a76 into trunk May 13, 2021

kangzj deleted the fix/instant-search-toggle branch May 13, 2021 03:57

github-actions bot removed the [Status] Ready to Merge label May 13, 2021

kangzj mentioned this pull request May 14, 2021

Jetpack Search: Make instructions clearer for Business plan Instant Search toggle #52853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetpack Search: instant search toggle should change with Jetpack Search toggle #52631

Jetpack Search: instant search toggle should change with Jetpack Search toggle #52631

kangzj commented May 6, 2021 •

edited

Loading

github-actions bot commented May 6, 2021

matticbot commented May 6, 2021 •

edited

Loading

kangzj commented May 6, 2021 •

edited

Loading

bluefuton commented May 11, 2021

bluefuton commented May 11, 2021

kangzj commented May 11, 2021

kangzj commented May 12, 2021

jsnmoon left a comment •

edited

Loading

kangzj commented May 13, 2021 •

edited by jsnmoon

Loading

bluefuton May 13, 2021

bluefuton left a comment

kangzj commented May 13, 2021

a8ci18n commented May 13, 2021

jsnmoon commented May 13, 2021

kangzj commented May 13, 2021

cavalierlife commented May 18, 2021

kangzj commented May 18, 2021

a8ci18n commented May 21, 2021

Jetpack Search: instant search toggle should change with Jetpack Search toggle #52631

Jetpack Search: instant search toggle should change with Jetpack Search toggle #52631

Conversation

kangzj commented May 6, 2021 • edited Loading

Changes proposed in this Pull Request

Discussion

Testing instructions

github-actions bot commented May 6, 2021

matticbot commented May 6, 2021 • edited Loading

kangzj commented May 6, 2021 • edited Loading

bluefuton commented May 11, 2021

bluefuton commented May 11, 2021

kangzj commented May 11, 2021

kangzj commented May 12, 2021

jsnmoon left a comment • edited Loading

Choose a reason for hiding this comment

kangzj commented May 13, 2021 • edited by jsnmoon Loading

bluefuton May 13, 2021

Choose a reason for hiding this comment

bluefuton left a comment

Choose a reason for hiding this comment

kangzj commented May 13, 2021

a8ci18n commented May 13, 2021

jsnmoon commented May 13, 2021

kangzj commented May 13, 2021

cavalierlife commented May 18, 2021

kangzj commented May 18, 2021

a8ci18n commented May 21, 2021

kangzj commented May 6, 2021 •

edited

Loading

matticbot commented May 6, 2021 •

edited

Loading

kangzj commented May 6, 2021 •

edited

Loading

jsnmoon left a comment •

edited

Loading

kangzj commented May 13, 2021 •

edited by jsnmoon

Loading