Skip to content

Search: Add setting that triggers running the instant search auto config.#15186

Merged
gibrown merged 2 commits intomasterfrom
add/search-auto-confing-endpoint
Mar 30, 2020
Merged

Search: Add setting that triggers running the instant search auto config.#15186
gibrown merged 2 commits intomasterfrom
add/search-auto-confing-endpoint

Conversation

@gibrown
Copy link
Copy Markdown
Member

@gibrown gibrown commented Mar 30, 2020

Adds a way to trigger the auto config of search so that we can have a smooth default onboarding experience.

I've only done minimal testing so far, but it did work. I'm also not sure this is the best way to go. I especially didn't retest some of the corner cases: https://github.com/Automattic/jetpack/blob/master/modules/search/class-jetpack-instant-search.php#L391

Testing:

  • start with a site on a free plan, a sidebar theme (e.g. 2016), and with no jp search widgets configured in the sidebar
  • run D41051 on your sandbox and sandbox the public api
  • run dev calypso (unless Jetpack Search: Search plan to staging for testing the full flow. wp-calypso#40545 has landed)
  • Purchase a search plan
  • Your site should have the search widgets get added to both the main sidebar and the overlay sidebar

Fast CL testing:

  • see D41051

@gibrown gibrown added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Needs Review This PR is ready for review. [Feature] Search For all things related to Search Instant Search labels Mar 30, 2020
@gibrown gibrown added this to the InstantSearchLaunch milestone Mar 30, 2020
@gibrown gibrown requested review from a team, AnnaMag, jeherve and jsnmoon March 30, 2020 02:41
@gibrown gibrown self-assigned this Mar 30, 2020
@gibrown gibrown mentioned this pull request Mar 30, 2020
3 tasks
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Mar 30, 2020

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 1fa23b8

Copy link
Copy Markdown
Contributor

@mdbitz mdbitz left a comment

Choose a reason for hiding this comment

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

Marking this Approved.

The endpoint tests out for me and does trigger the auto-config code. More testing is needed of the auto-config flows but I'm good with this moving ahead as the trigger.

@dereksmart @jeremeylduvall what do you think of this approach to triggering the auto-config?

Copy link
Copy Markdown
Contributor

@AnnaMag AnnaMag left a comment

Choose a reason for hiding this comment

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

I'd suggest refactor to mimic the onboarding endpoint, the use of the widget library and improved error handling in the follow up. LGTM as a v1.

@gibrown gibrown merged commit 8735119 into master Mar 30, 2020
@gibrown gibrown deleted the add/search-auto-confing-endpoint branch March 30, 2020 22:20
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review This PR is ready for review. labels Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Search For all things related to Search

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants