Skip to content

Conversation

gibrown
Copy link
Member

@gibrown gibrown commented Aug 30, 2019

This is the same as #13332 but merging into instant-search-master rather than master.

Adds the display of filters to the search widget. Still a fair bit of work to do here, but trying to merge it into the feature branch so I can use the better API code in other places (sorting for instance).

@gibrown gibrown requested review from jsnmoon and a team August 30, 2019 15:01
@gibrown gibrown self-assigned this Aug 30, 2019
@gibrown
Copy link
Member Author

gibrown commented Aug 30, 2019

OK, I think I got all of this working now.

@jsnmoon I decided to just pass the whole options object into SearchWidget because it felt like we should do the checking for whether or not options exist inside the app. Have to handle cases where there is no search widget configured in which case there will be no aggregations or widgets defined. I think I did this in a reasonable way...

Screen Shot 2019-08-30 at 10 09 07 AM

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, left a small comment that we can address in a future polish PR; should be good to go after a quick lookover from @bluefuton.

this.requestId = 0;
this.props.resultFormat = 'minimal';
this.props.aggregations = buildFilterAggregations( this.props.options.widgets );
this.props.widgets = this.props.options.widgets ? this.props.options.widgets : [];
Copy link
Contributor

@jsnmoon jsnmoon Sep 3, 2019

Choose a reason for hiding this comment

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

It's probably not a big deal since we're only doing this once in the constructor, but React Props are meant to be read-only.

Perhaps it'd be better to use a memoized accessor method like SearchApp.getAggregations? Or directly reference this.props.options.widgets when we need to access values from within options.

We can leave this alone for now, I think.

@bluefuton
Copy link
Contributor

It'd be good to format the date filter label according to the period selected (dropping the time in all cases), but we can change that later 👍

@gibrown gibrown merged commit eedd282 into instant-search-master Sep 4, 2019
@gibrown gibrown deleted the add/filtering-for-instant-search branch September 4, 2019 15:10
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review This PR is ready for review. labels Sep 4, 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 Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants