Filter Products By Price block: don't allow to insert negative values on inputs #5123
Conversation
… Price block #2695 Don't allow to insert negative values on input for Filter Products By Price block
gigitux
added
type: bug
rubikuserbot
requested review from
a team and
frontdevde
and removed request for
a team
|
Size Change: +290 B (0%) Total Size: 819 kB
ℹ️ View Unchanged
|
| @@ -17,6 +17,8 @@ import './style.scss'; | |||
| interface FormattedMonetaryAmountProps { | |||
| className?: string; | |||
| displayType?: NumberFormatProps[ 'displayType' ]; | |||
| allowNegative?: boolean; | |||
| isAllowed?: ( formattedValue: NumberFormatValues ) => boolean; | |||
There was a problem hiding this comment.
isAllowed is rather confusing, what are we supposed to allow here? what I understood from the PR is that it doesn’t allow negative numbers, for me allowNegative should be sufficient.
Also, do we need to pass a prop to FormattedMonetaryAmount just to support negative numbers? shouldn't just support them by default from what we pass to value?
There was a problem hiding this comment.
isAllowed is rather confusing, what are we supposed to allow here? what I understood from the PR is that it doesn’t allow negative numbers, for me allowNegative should be sufficient.
I used isAllowed because with the function withMaxValueLimit and withMinValueLimit we can validate the input when the user changes the value in the input. About the naming, I chose isAllowed to be consistent with the props of the library react-number-format.
Also, do we need to pass a prop to FormattedMonetaryAmount just to support negative numbers? shouldn't just support them by default from what we pass to value?
Do you mean that we should implement some custom logic to check if the value is negative? I preferred to use a ready solution that react-number-format give
There was a problem hiding this comment.
I still don't understand the goal of isAllowed, can't allowNegative do that instead? My comment is mostly why we need two.
There was a problem hiding this comment.
The goal isAllowed is to do an extra check on the input field:
If on the right input the user write 30, on the left input the user can't write 31 (and this is the goal of the functions withMinValueLimit and withMaxValueLimit)
There was a problem hiding this comment.
It took me sometime to understand isAllowed. I initially thought this is a prop you introduced, I see it's part of react-format-money, Not a great naming from their side really, but I see its value now.
| // When user insert in the minimum price input a value greater than the max price, | ||
| // we update the value of the slider to 0 - max. |
There was a problem hiding this comment.
Can't we just reject the min inserted and default to the previous min instead of reseting max as well? this is more a UX thing and not code related but if something changes, just reset it instead of reseting two things.
There was a problem hiding this comment.
Can't we just reject the min inserted and default to the previous min instead of reseting max as well? this is more a UX thing and not code related but if something changes, just reset it instead of reseting two things.
My comment is probably unclear. For max, I mean the current max price. So what my code does is just reset to 0 the minimum price.
What do you think about this:
| // When user insert in the minimum price input a value greater than the max price, | |
| // we update the value of the slider to 0 - max. | |
| // When the user inserts in the minimum price input a value greater than the max price, | |
| // we set to 0 the minimum price. |
There was a problem hiding this comment.
I rephrased it again because I just re-checked the logic better.
// When the user inserts in the max price input a value less or equal than the current minimum price,
// we set to 0 the minimum price.
| maxConstraint: number; | ||
| minorUnit: number; | ||
| } ) => ( { floatValue }: NumberFormatValues ): boolean => { | ||
| const maxPrice = maxConstraint / 10 ** minorUnit; |
There was a problem hiding this comment.
We probably don't want to use this logic here given that we had issues with it, preferably we just deal with raw versions 1000 and let the final UI element handle formatting.
There was a problem hiding this comment.
let the final UI element handle formatting.
This is code is not about formatting the number, but it is just about validation.
The code checks that on the right input the user can't insert a number that is minor then the number that there is in the left input. About the naming of the function, what do you think about checkMaxValueLimit?
There was a problem hiding this comment.
Given that withMaxValueLimit is a boolean function, anything with has_ or is_ should work. Maybe isOutsideBound? Naming is hard so don't stress it.
| return floatValue !== undefined && floatValue <= maxPrice; | ||
| }; | ||
|
|
||
| export const withMinValueLimit = ( { |
There was a problem hiding this comment.
A doc block explaining the goal of this function would be great.
with prefix is usually used for HOCs, I find it rather confusing here. The logic in the return is also confusing from a first look and probably deserves to be dumped down or explained.
There was a problem hiding this comment.
| export const withMinValueLimit = ( { | |
| // Check if that the value is greater than the lowest available price and minor than the current max price. | |
| export const checkMinValueLimit = ( { |
What do you think?
There was a problem hiding this comment.
Yes! Much better, would also add a comment to withMaxValueLimit.
…products-block into fix/2695-not-allow-negative-values-filter-products-by-price
gigitux
deleted the
fix/2695-not-allow-negative-values-filter-products-by-price
branch
This PR fixes some not unexpected behavior for the Filter Products By Price block:
The user can insert a negative value in the inputs
Now, with this PR, the user can't insert a negative value. This is possible thanks to the prop
allowNegativesetted totrue.The user can "break" the slider when inserting in the right input a value that is lower than the value on the left input
Now, with this PR, when the user inserts in the right input a value that is lower than the value on the left input, the latter is reset to 0.
This check is done thanks to
isValidMinValueandisValidMaxValueand this condition.Fixes #2695
Testing
Manual Testing
How to test the changes in this Pull Request:
Check out this branch
Check that:
Changelog