-
Notifications
You must be signed in to change notification settings - Fork 530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ratingMenu): Add support for floats in values #4611
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d94781a:
|
@@ -129,31 +132,77 @@ export default function connectRatingMenu(renderFn, unmountFn = noop) { | |||
checkRendering(renderFn, withUsage()); | |||
|
|||
return (widgetParams = {}) => { | |||
const { attribute, max = 5 } = widgetParams; | |||
const { attribute, max = 5, step = 1 } = widgetParams; |
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.
I compared https://codesandbox.io/s/instantsearchjs-forked-ttnb0 (previous) and https://codesandbox.io/s/instantsearchjs-forked-92pyp?file=/src/app.js (with this PR) and the number of results changed, even without changing the step. Should we have a default that allows only integer? I'm not sure what step means. If I set it to 1, the warning still shows up, and 2 I think it rounds oddly?
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.
The new implementation was keeping the values >= to max
into the count, while the previous implementation was ignoring them. I fixed the calculation to match the previous implementation.
The step
parameter changes how the results get visually grouped, for example if you want to display half-stars you can set the step to 0.5.
Grouping with step
set to 1
(default):
>= 4
>= 3
>= 2
>= 1
Grouping with step
set to 0.5
:
>= 4.5
>= 4
>= 3.5
>= 3
>= 2.5
>= 2
>= 1.5
>= 1
>= 0.5
Changing the grouping does not affect the warning, the only solutions I found to get the correct count (and therefore to fix the warning) is to lower the precision of the values (less possible facet values) and/or to retrieve more facets (to get all possible values).
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.
I've updated both sandboxes and the count is now more reasonable, but neither really work as expected (saying ~100 hits, while really there are thousands, in the previous implementation 1 hit).
Since the hits are so different, I think this is a breaking change (unfortunately)
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.
I updated the implementation to apply the maximum value in the refinement.
eg. you add a ratingMenu with a maximum value of 5 and apply a refinement on 3 and up, you'll get numericFilters: ["price<=5","price>=3"]
. The count is accurate then.
You can still have some small imprecision on the count but I think this is due to engine limitations (to confirm).
…s into feat/rating-menu
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.
This is the last main issue imho. As we saw together it is not possible to get the correct count if you reach the limit of possible facet values. With the previous implementation the count was obviously incorrect, now with this implementation the count is "more correct", even if not exact. Do we consider this change of behavior as a breaking change or not? For me it is closer to a fix than something else (since the previous behavior was incorrect), therefore if should not be considered as a breaking change. (the above statement assume that the change from a |
I think for the moment, since the existing behaviour in this cases (floats) was very wrong, and it's better now, without significant impact on non-float ratings, we should put a note in the changelog, but it should be fine for the rest! |
Haven't checked this PR lately. Can you briefly explain to me (or, give me the line numbers) how it was broken and how it's fixed? |
Before this change we were only handling integers in the rating menu. In the previous implementation there was 2 main issues:
The first issue was fixed by not doing this rounding anymore (you can see how the new The second one by using a Let me know is something is not clear and/or if the code could use some comments on a particular part 🙂 |
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.
@yannickcr super clear. Thanks for the explanation.
The PR looks good to me.
@Haroenv I removed the |
Summary
This PR adds the support for floats to the ratingMenu connector.
Main changes
step
option, so the user can control the step between ratings (example:1
=>1, 2, 3, 4, 5
or0.5
=>0.5, 1, 1.5, 2, 2.5, 3, 3.5, 4, 4.5, 5
). Default to1
so the original behavior is unchanged.facetValues
are calculated in the connector, taking thisstep
option into account.numericRefinements
instead ofdisjunctiveFacetsRefinements
since it is easier to specify a complete range of values (example:>=3
) instead of being forced to specify all possible values manually (example:[3, 4, 5]
) which can be very big if the range is large and/or if the steps are small (example:[3, 3.1, 3.2, 3.3, 3.4, etc.]
). FMPOV it should not be a breaking change, but if this is the case we can still revert todisjunctiveFacetsRefinements
(even if that's way less optimal IMHO).Maximum facets values limitation
One big issue with the floats support is that the user is more likely to quickly hit the limit of retrievable facets values (limited to 1000 by the API).
For example imagine you have a rating from 0 to 10 with 2 decimals (example:
2.56, 3.78, 6.48, etc.
). It means you have 10 integers and 100 values per integer = 1000 possible facets values. You reached the limit.To avoid this issue as much as possible we need to guide the user so he can update his records and/or his configuration accordingly.
So if we detect that we enter this case where we did not retrieved enough facets the widget will display the following warning message:
This purpose of this message is to explain what is wrong in the current configuration, what are the consequences and to give some solutions to solve the problem.
Any ideas to improve this error message are welcome 😉