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

Instant Search: fix many theme incompatibilities #13602

Merged
merged 10 commits into from
Oct 8, 2019

Conversation

gibrown
Copy link
Member

@gibrown gibrown commented Sep 30, 2019

Note: this merges into the instant-search-master branch

Fixes many of the problems in #13391 particularly:

  • inserting results into the theme works in all those themes
  • the search form for all those themes works as search as you type as does the default core WP search widget
  • Adds some CSS for our search box and cleans it up
  • Adjust opacity when results are loading to give the user some feedback
  • Starts the search query after only 200ms rather than 500ms. Feels a lot snappier
  • Did some slight code cleanup and renamed the "widget" to "app" so it isn't confused with the WP widgets.

Testing instructions:

  • Add define( "JETPACK_SEARCH_PROTOTYPE", true ); to your wp-config.php.
  • Ensure that your site has the Jetpack Pro plan and has Jetpack Search enabled.
  • Add a Jetpack Search widget to the Search page sidebar. (though I guess it should now work without this added at all)
  • Enter a query into a search widget. Alternatively, navigate to a search page like /?s=privacy.
  • You can also test against any other site by adding this filter and then adding &blog_id=20115252 to your search page url (that is the jetpack.com blog_id).
function jp_instant_search_options( $options ) {
	if ( $_GET['blog_id'] ) {
		$options['siteId'] = (int) $_GET['blog_id'];
	}
	return $options;
}
add_filter( 'jetpack_instant_search_options', 'jp_instant_search_options' );

Proposed changelog entry for your changes:

  • None

@gibrown gibrown added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Feature] Search For all things related to Search Instant Search labels Sep 30, 2019
@gibrown gibrown requested a review from a team September 30, 2019 17:54
@gibrown gibrown self-assigned this Sep 30, 2019
@gibrown gibrown requested a review from bluefuton October 1, 2019 16:01
@gibrown
Copy link
Member Author

gibrown commented Oct 1, 2019

I think I got everything from the review done. I just deleted the -moz border stuff assuming that is what you meant.

@bluefuton
Copy link
Contributor

@gibrown is there a target list of themes I should test for this PR?

@gibrown
Copy link
Member Author

gibrown commented Oct 1, 2019

@bluefuton #13391 has the list that i tried with.

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.

Looks good overall.

The only thing that really concerns me is dropping IE 6-10 support by using Intl.NumberFormat, which I noted inline below.

@@ -0,0 +1,11 @@
label.jp-instant-search__box input:focus {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like an unused style -- delete?

@jsnmoon
Copy link
Contributor

jsnmoon commented Oct 2, 2019

Just wanted to note for future reviewers:

🙂 2010, 2011, 2012, 2014, 2016, Astra, Colormag, Storefront, Sydney, look decent.

😶Filter checkboxes require additional spacing for most themes.

😦 2013 and 2019 don't look quite right since the widget content is shown in the footer.
🙁 2015 needs a styled container for the search results.
😦 Argent suffers similarly to 2013 and 2019 due to its layout lacking a sidebar area.
😦 Hemingway unnecessarily spawns two unstyled search inputs below the search widget.
🙁 Shapely has unnecessary left margins for the filter checkboxes.
😦 Zerif Lite unnecessarily centers search results and sets the width of the filter checkbox labels to 100%.

@kraftbj
Copy link
Contributor

kraftbj commented Oct 3, 2019

2010, 2011, 2012, 2014, 2016, ... look decent.

2020 is almost done and likely worth checking out too.

@simison
Copy link
Member

simison commented Oct 3, 2019

BTW I made a tool to test several themes simultaneously, you might find it useful: https://wordpress.org/plugins/back-to-the-theme/

@gibrown gibrown requested a review from jsnmoon October 4, 2019 10:37
@gibrown
Copy link
Member Author

gibrown commented Oct 5, 2019

Easy way to override the theme by specifying the slug in the url. e.g. http://gibrown.wpsandbox.me/main/?s=related%20psts&blog_id=20115252&theme=twentyseventeen

function filter_theme( $theme ) {
	if ( $_GET['theme'] ) {
		$theme = sanitize_key( $_GET['theme'] );
	}
	return $theme;
}
add_filter( 'stylesheet', 'filter_theme', 1 );
add_filter( 'template', 'filter_theme', 1 );

@gibrown gibrown merged commit bbc501e into instant-search-master Oct 8, 2019
@gibrown gibrown deleted the add/instant-search-theme-support branch October 8, 2019 00:08
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Oct 8, 2019
jsnmoon added a commit that referenced this pull request Oct 23, 2019
* Implement minimal search results and spelling correction (#13365)
* Add filtering display (#13371)
* Fix search result display bugs and make improvements (#13393)
* Add rudimentary support for filtering on post types (#13430)
* Add support for filtering on categories and tags (#13505)
* Add instant search sorting based on the URL (#13377)
* Add support for filtering on dates (#13545)
* Add custom taxonomy filtering (#13605)
* add sort widget (#13614)
* fix many theme incompatibilities (#13602)
* Add infinite scrolling (#13684)
* Add caching to the api requests (#13714)
* Clean up some design bugs/issues (#13721)
* Fix labels for post types when we have them. (#13750)
* Add localization and formatting of all dates (#13748)
* search from any page on the site (#13713)
* Hook up default options (inc. sort) (#13742)
* Add TrainTracks analytics (#13730)
* Create PostTypeIcon component (#13790)
* Upgrade to Preact 10 (#13794)
* Add comments component (#13797)
* Address review feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Search For all things related to Search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants