This repository has been archived by the owner on Jul 11, 2023. It is now read-only.
feat(getFacetValues): process facetOrdering #822
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
Haroenv
commented
Jun 9, 2021
Haroenv
force-pushed
the
feat/facet-value-ordering
branch
from
e464183
to
a620e5b
Compare
Haroenv
requested review from
a team,
tkrugg and
shortcuts
and removed request for
a team
shortcuts
reviewed
Jun 10, 2021
Comment on lines
+736
to
+738
| } else { | ||
| ordering = [['count'], ['desc']]; | ||
| } |
There was a problem hiding this comment.
Making sure: we decide to fallback to sorting by count and not throwing if the provided order is wrong?
There was a problem hiding this comment.
it's something that's a bit blurry at the moment, the spec mentions falling back to sortFacetValuesBy, but I can't retrieve that in the result-side, so I think falling back to just count by default is reasonable.
Alternatively we could make sortRemainingBy required in the dashboard possibly. What did you pick @VladislavFitz?
There was a problem hiding this comment.
required in the dashboard
What do you mean? Make the engine always return renderingContent even if it's not set?
There was a problem hiding this comment.
this is about the key sortRemainingBy, what do you sort by if the key isn't given?
There was a problem hiding this comment.
Sort by count by default sounds good to me 👍
Comment on lines
+164
to
+167
| var facetValues = result.getFacetValues('brand', { | ||
| sortBy: ['name:desc'], | ||
| facetOrdering: true | ||
| }); |
There was a problem hiding this comment.
That behavior is getting confusing. Shouldn't we throw instead?
There was a problem hiding this comment.
this is needed for the hierarchical menu and menu widgets, they have a sortBy which isn't the default one, but still need to take facet ordering in account.
There was a problem hiding this comment.
IIUC that's an internal constraint but that constraint leaks in the public API, which is annoying—but I get the point.
There was a problem hiding this comment.
I can see how others might want both a non-default sortBy, but still try to use facetOrdering if it's given
francoischalifour
approved these changes
Jun 14, 2021
Haroenv
added a commit
to algolia/react-instantsearch
that referenced
this pull request
Jun 25, 2021
…mentList & HierarchicalMenu If `facetOrdering` is enabled (the default behaviour), before the default sortBy is used, the result from renderingContent.facetOrdering.values is first checked. If that's present, it will be used to sort the items. You can still change that ordering afterwards with the existing transformItems, so if you are sorting in transformItems, you actually override the sorting done by facet ordering, and won't see the effect. To use facetOrdering, you thus need to remove any sorting done in transformItems. If there is a facetOrdering present in the index, but you don't want to use it for a certain widget, you need to explicitly pass `facetOrdering: false` to the widget or connector References: - [RFC 45](https://github.com/algolia/instantsearch-rfcs/blob/master/accepted/flexible-facet-values.md) - algolia/instantsearch#4784 - algolia/algoliasearch-helper-js#822
Haroenv
added a commit
to algolia/react-instantsearch
that referenced
this pull request
Jun 28, 2021
…mentList & HierarchicalMenu If `facetOrdering` is enabled (the default behaviour), before the default sortBy is used, the result from renderingContent.facetOrdering.values is first checked. If that's present, it will be used to sort the items. You can still change that ordering afterwards with the existing transformItems, so if you are sorting in transformItems, you actually override the sorting done by facet ordering, and won't see the effect. To use facetOrdering, you thus need to remove any sorting done in transformItems. If there is a facetOrdering present in the index, but you don't want to use it for a certain widget, you need to explicitly pass `facetOrdering: false` to the widget or connector References: - [RFC 45](https://github.com/algolia/instantsearch-rfcs/blob/master/accepted/flexible-facet-values.md) - algolia/instantsearch#4784 - algolia/algoliasearch-helper-js#822
Haroenv
added a commit
to algolia/react-instantsearch
that referenced
this pull request
Jun 30, 2021
…mentList & HierarchicalMenu If `facetOrdering` is enabled (the default behaviour), before the default sortBy is used, the result from renderingContent.facetOrdering.values is first checked. If that's present, it will be used to sort the items. You can still change that ordering afterwards with the existing transformItems, so if you are sorting in transformItems, you actually override the sorting done by facet ordering, and won't see the effect. To use facetOrdering, you thus need to remove any sorting done in transformItems. If there is a facetOrdering present in the index, but you don't want to use it for a certain widget, you need to explicitly pass `facetOrdering: false` to the widget or connector References: - [RFC 45](https://github.com/algolia/instantsearch-rfcs/blob/master/accepted/flexible-facet-values.md) - algolia/instantsearch#4784 - algolia/algoliasearch-helper-js#822
Haroenv
added a commit
to algolia/react-instantsearch
that referenced
this pull request
Jul 2, 2021
…mentList & HierarchicalMenu If `facetOrdering` is enabled (the default behaviour), before the default sortBy is used, the result from renderingContent.facetOrdering.values is first checked. If that's present, it will be used to sort the items. You can still change that ordering afterwards with the existing transformItems, so if you are sorting in transformItems, you actually override the sorting done by facet ordering, and won't see the effect. To use facetOrdering, you thus need to remove any sorting done in transformItems. If there is a facetOrdering present in the index, but you don't want to use it for a certain widget, you need to explicitly pass `facetOrdering: false` to the widget or connector References: - [RFC 45](https://github.com/algolia/instantsearch-rfcs/blob/master/accepted/flexible-facet-values.md) - algolia/instantsearch#4784 - algolia/algoliasearch-helper-js#822
Haroenv
added a commit
to algolia/react-instantsearch
that referenced
this pull request
Jul 5, 2021
…t & HierarchicalMenu (#3067) * feat(facetOrdering): add a new option "facetOrdering" to Menu, RefinementList & HierarchicalMenu If `facetOrdering` is enabled (the default behaviour), before the default sortBy is used, the result from renderingContent.facetOrdering.values is first checked. If that's present, it will be used to sort the items. You can still change that ordering afterwards with the existing transformItems, so if you are sorting in transformItems, you actually override the sorting done by facet ordering, and won't see the effect. To use facetOrdering, you thus need to remove any sorting done in transformItems. If there is a facetOrdering present in the index, but you don't want to use it for a certain widget, you need to explicitly pass `facetOrdering: false` to the widget or connector References: - [RFC 45](https://github.com/algolia/instantsearch-rfcs/blob/master/accepted/flexible-facet-values.md) - algolia/instantsearch#4784 - algolia/algoliasearch-helper-js#822 * clarify
Haroenv
added a commit
to algolia/instantsearch
that referenced
this pull request
Jan 4, 2023
…t & HierarchicalMenu (algolia/react-instantsearch#3067) * feat(facetOrdering): add a new option "facetOrdering" to Menu, RefinementList & HierarchicalMenu If `facetOrdering` is enabled (the default behaviour), before the default sortBy is used, the result from renderingContent.facetOrdering.values is first checked. If that's present, it will be used to sort the items. You can still change that ordering afterwards with the existing transformItems, so if you are sorting in transformItems, you actually override the sorting done by facet ordering, and won't see the effect. To use facetOrdering, you thus need to remove any sorting done in transformItems. If there is a facetOrdering present in the index, but you don't want to use it for a certain widget, you need to explicitly pass `facetOrdering: false` to the widget or connector References: - [RFC 45](https://github.com/algolia/instantsearch-rfcs/blob/master/accepted/flexible-facet-values.md) - #4784 - algolia/algoliasearch-helper-js#822 * clarify
dhayab
pushed a commit
to algolia/instantsearch
that referenced
this pull request
Jul 10, 2023
…lper-js#822) * feat(getFacetValues): process facetOrdering DX-2075 * fix: behave correctly on facetOrdering: false * pinned -> ordered * better description * simplify tests & code slightly
dhayab
pushed a commit
to algolia/instantsearch
that referenced
this pull request
Jul 10, 2023
* feat(getFacetValues): process facetOrdering (algolia/algoliasearch-helper-js#822) algolia/algoliasearch-helper-js@6fca6be
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.
This adds support for reading from facetOrdering in getFacetValues. It's applied by default if no sortBy is given, but can be forced (or disabled) by
facetOrderingin opts.DX-2075