Instant Search: Render disabled filter widgets#13332
Instant Search: Render disabled filter widgets#13332
Conversation
|
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: September 3, 2019. |
gibrown
left a comment
There was a problem hiding this comment.
Looking good. We could hold off on the filter results formatting, etc for another PR given the current size. Also there is some overlap with my PR and merging this could help us clean that up. I think the two blockers are that the search box is disappearing for me and the asset size stuff.
| type="checkbox" | ||
| /> | ||
| <label htmlFor={ `jp-instant-search-filter-dates-${ bucket.key_as_string }` }> | ||
| { strip( bucket.key_as_string ) } ({ bucket.doc_count }) |
There was a problem hiding this comment.
This should get localized and we should remove the hours, minutes, secs. There is some config in the date config that indicates if we are displaying months or years.
There was a problem hiding this comment.
When you say date config, are you referring to a query value that could be specified in the API request?
There was a problem hiding this comment.
Ya there is interval information from the widget filter config: https://cloudup.com/cD3amQW3Ba1
That should be applied both for the API request and for determining how to display the results (if year, then only show the year, if month, then show year and month).
There was a problem hiding this comment.
Could we push the interval formatting and localization to the API? Otherwise, we'll need to consider adding a date formatting library to the client, which could add a minimum of 15kB to our production bundle.
The interval value is already being passed in the API call.
There was a problem hiding this comment.
Possibly ya. Is the 15kb from pulling in https://www.npmjs.com/package/@wordpress/date which looks like it includes date_i18n()? We may want something like this anyways to correctly display in search results though.
| ] | ||
| // TODO: Remove this reverse & slice when API adds filter count support | ||
| .reverse() | ||
| .slice( 0, 5 ) } |
There was a problem hiding this comment.
I think the count comes from the config.
There was a problem hiding this comment.
The API currently throws a 400 when I specify a size query value for aggregations[date_histogram_0][date_histogram][size]. See this example query.
There was a problem hiding this comment.
Ya, the api will always return all of them, but we have an option (https://cloudup.com/cD3amQW3Ba1) that lets the user indicate how many to show. 5 shouldn't be hard coded.
There was a problem hiding this comment.
Is it possible to support the size query value as it does for taxonomy and post_type aggregations?
In the meanwhile, I'll look into linking up the widget configuration values to the aggregation values in the client in a follow-up PR.
There was a problem hiding this comment.
ES doesn't support it itself, so we would have to post process is somehow on the API side. I'd rather not deviate from the ES standards for aggs and filters though as that could make things harder to support long term.
|
@jsnmoon since we are going to try a separate branch to merge into, maybe what is in here is good to go and you should just make a new PR against that branch, I or @bluefuton can cast some eyes over it and then we can handle the bug and display fixes in separate PRs? |
…ion (#13365) Refactor search results and improve their display. Add display of when we make spelling corrections.
…earch Everything seems to work.
|
I have opened this PR against the |
|
We'll be continuing this work in #13371 and landing our work in a feature branch instead of in |
Changes proposed in this Pull Request:
When a user performs a search using a Jetpack Search widget, the search page will now include a dynamically generated list of filters on the sidebar like so:
The checkbox inputs are currently disabled at this time. Instant Search remains gated by a
JETPACK_SEARCH_PROTOTYPEdefine.Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
define( "JETPACK_SEARCH_PROTOTYPE", true );to your wp-config.php.Alternatively, navigate to a search page like
/?s=privacy.Proposed changelog entry for your changes: