-
Notifications
You must be signed in to change notification settings - Fork 3
feat(CurrentRefinements): add query and remove reset #86
Conversation
|
Deploy preview for instantsearch-css ready! Built with commit a5a3c17 |
francoischalifour
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the excludedAttributes array is used to filter includedAttributes, and then the whole thing is transformed with transformItems.
|
Yes, that makes sense to me. We can move to the better descriptions once we did the API ref docs |
| - name: excludedAttributes | ||
| description: Attributes not to clear | ||
| - name: includedAttributes | ||
| description: Attributes to show exclusively |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have this one on CurrentRefinements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have it for both ClearRefinements (algolia/instantsearch#3114) and CurrentRefinements (yet to be implemented).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep but it's not written in the specs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Will add it!
| - name: excludedAttributes | ||
| description: list of attributes not to show | ||
| - name: clearsQuery | ||
| - name: includesQuery |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now have two different option names includesQuery and clearsQuery?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can rename it to includesQuery in ClearRefinements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it feels a bit redundant to have excludes, includes and includesQuery. It could be implemented as includes with a wildcard for example (probably not the best though - see below). An other solution is to have the value of the query present by default in CurrentRefinements & ClearRefinements.
includes = ['*', 'query']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a new pattern in InstantSearch I think?
I reckon it's a good compromise. Maybe calling this option attributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be confusing because that’s the previous option name no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should still be able to excludes & includes with only attributes we can't. I agree with @Haroenv on the question about the name of the options. attributes will be a bit confusing regarding the other widgets.
…search-specs#86) * feat(CurrentRefinements): add query and remove reset * feat: includedAttributes
…search-specs#86) * feat(CurrentRefinements): add query and remove reset * feat: includedAttributes
…search-specs#86) * feat(CurrentRefinements): add query and remove reset * feat: includedAttributes
https://deploy-preview-86--instantsearch-css.netlify.com/widgets/current-refinements/