This repository was archived by the owner on Dec 30, 2022. It is now read-only.
feat(CurrentRefinements): consolidate API #550
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
query state should always go first, since that way other widgets are able to control the query
samouss
suggested changes
Oct 18, 2018
| return { | ||
| transformItems: items => | ||
| items.map(item => { | ||
| item.label = item.label.toLocaleUpperCase(); |
There was a problem hiding this comment.
Prefer create a new object rather than mutate the value.
francoischalifour
suggested changes
Oct 18, 2018
| * "type": which can be "facet", "exclude", "disjunctive", "hierarchical", "numeric" or "query" | ||
| * "attribute": used as the key | ||
| * "label": string form of the value | ||
| * "value": necessary for the refinement to work correctly, no need to be changed |
There was a problem hiding this comment.
Suggested change
| * "value": necessary for the refinement to work correctly, no need to be changed | |
| * `value`: necessary for the refinement to work correctly, no need to be changed |
|
|
||
| * "type": which can be "facet", "exclude", "disjunctive", "hierarchical", "numeric" or "query" | ||
| * "attribute": used as the key | ||
| * "label": string form of the value |
There was a problem hiding this comment.
Suggested change
| * "label": string form of the value | |
| * `label`: string form of the value |
| an Item has the keys: | ||
|
|
||
| * "type": which can be "facet", "exclude", "disjunctive", "hierarchical", "numeric" or "query" | ||
| * "attribute": used as the key |
There was a problem hiding this comment.
Suggested change
| * "attribute": used as the key | |
| * `attribute`: used as the key |
|
|
||
| an Item has the keys: | ||
|
|
||
| * "type": which can be "facet", "exclude", "disjunctive", "hierarchical", "numeric" or "query" |
There was a problem hiding this comment.
Suggested change
| * "type": which can be "facet", "exclude", "disjunctive", "hierarchical", "numeric" or "query" | |
| * `type`: which can be "facet", "exclude", "disjunctive", "hierarchical", "numeric" or "query" |
| `item` | `{ item: Item, refine: Item => void }` | Override how an item looks | ||
| `clearAllLabel` | `{ items: Item[] }` | Override how the "clear all" button looks | ||
|
|
||
| an Item has the keys: |
There was a problem hiding this comment.
Suggested change
| an Item has the keys: | |
| An item has the keys: |
There was a problem hiding this comment.
On purpose, the name of the type is Item. Will put it in backticks
| }); | ||
|
|
||
| const transformItems = items => | ||
| items.filter(({ attribute }) => attribute === 'query'); |
There was a problem hiding this comment.
This doesn't seem to test if transformItems is applied after excludedAttributes.
A way to effectively test it would be to overwrite the whole content with transformItems.
| ); | ||
| }); | ||
|
|
||
| it("TransformItems happens after excludedAttributes (so it doesn't include query by default", () => { |
There was a problem hiding this comment.
Suggested change
| it("TransformItems happens after excludedAttributes (so it doesn't include query by default", () => { | |
| it("TransformItems happens after excludedAttributes (so it doesn't include query by default)", () => { |
samouss
reviewed
Oct 18, 2018
| clearsQuery: | ||
| this.excludedAttributes.indexOf('query') === -1 && | ||
| (this.includedAttributes | ||
| ? this.includedAttributes.indexOf('query') > -1 |
There was a problem hiding this comment.
Rather than > -1 I would go for !== -1 for consistency.
| expect(label.text()).toMatch(/Query/); | ||
| }); | ||
|
|
||
| it('includedAttributes overrides excludedAttributes 2', () => { |
There was a problem hiding this comment.
We can use a more meaningful name for the title of the test.
samouss
approved these changes
Oct 18, 2018
Haroenv
added a commit
to algolia/instantsearch
that referenced
this pull request
Dec 28, 2022
) * feat(current-refinements): consolidate API * chore(storybook): suppress Vue warning * fix(SearchBox): don't display internal value query state should always go first, since that way other widgets are able to control the query * docs(storybook): fix errors * feat(ClearRefinements): consistent naming * chore(ClearRefinements): lint * Update docs/src/components/ClearRefinements.md * Update docs/src/components/CurrentRefinements.md * Update docs/src/components/CurrentRefinements.md * Update CurrentRefinements.md * test: more focused test on transformItems being last * docs: fix typos & clarify * Update docs/src/components/CurrentRefinements.md * test: change naming * Update CurrentRefinements.vue
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
API of ClearRefinements & CurrentRefinements is consolidated
includedAttributes. This is waiting for v3 of InstantSearch.js and will be added without breaking changeincludedAttributestransparently takes precedence overexcludedAttributes.If you don't want to display specific attributes listed in
includedAttributes, don't write them or usetransformItems.This doesn't implement the "merged items" done in InstantSearch.js v3, which will be added later (we can do a major maybe)