Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Filter Products By Price block: don't allow to insert negative values on inputs #5123

Merged
merged 3 commits into from
Jan 6, 2022
Merged

Filter Products By Price block: don't allow to insert negative values on inputs #5123

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b51cf86
Don't allow to insert negative values on input for Filter Products By…
gigitux Nov 10, 2021
7201ce1
renaming util functions and add comments
gigitux Nov 19, 2021
3ca3001
Merge branch 'trunk' of github.com:woocommerce/woocommerce-gutenberg-…
gigitux Dec 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions assets/js/base/components/formatted-monetary-amount/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import './style.scss';
interface FormattedMonetaryAmountProps {
className?: string;
displayType?: NumberFormatProps[ 'displayType' ];
allowNegative?: boolean;
isAllowed?: ( formattedValue: NumberFormatValues ) => boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand the goal of isAllowed, can't allowNegative do that instead? My comment is mostly why we need two.

Copy link
Contributor Author

@gigitux gigitux Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

value: number | string; // Value of money amount.
currency: Currency | Record< string, never >; // Currency configuration object.
onValueChange?: ( unit: number ) => void; // Function to call when value changes.
Expand Down
29 changes: 29 additions & 0 deletions assets/js/base/components/price-slider/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { isObject } from '@woocommerce/types';
import './style.scss';
import { constrainRangeSliderValues } from './constrain-range-slider-values';
import FilterSubmitButton from '../filter-submit-button';
import { withMaxValueLimit, withMinValueLimit } from './utils';

/**
* Price slider component.
Expand Down Expand Up @@ -212,9 +213,27 @@ const PriceSlider = ( {
) {
return;
}

const isMin = event.target.classList.contains(
'wc-block-price-filter__amount--min'
);

// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@gigitux gigitux Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
// 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is great!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ( minPriceInput > maxPriceInput ) {
const values = constrainRangeSliderValues(
[ 0, maxPriceInput ],
null,
null,
stepValue,
isMin
);
return onChange( [
parseInt( values[ 0 ], 10 ),
parseInt( values[ 1 ], 10 ),
] );
}

const values = constrainRangeSliderValues(
[ minPriceInput, maxPriceInput ],
null,
Expand Down Expand Up @@ -322,6 +341,12 @@ const PriceSlider = ( {
'Filter products by minimum price',
'woo-gutenberg-products-block'
) }
allowNegative={ false }
isAllowed={ withMinValueLimit( {
minConstraint,
minorUnit: currency.minorUnit,
currentMaxValue: maxPriceInput,
} ) }
onValueChange={ ( value ) => {
if ( value === minPriceInput ) {
return;
Expand All @@ -340,6 +365,10 @@ const PriceSlider = ( {
'Filter products by maximum price',
'woo-gutenberg-products-block'
) }
isAllowed={ withMaxValueLimit( {
maxConstraint,
minorUnit: currency.minorUnit,
} ) }
onValueChange={ ( value ) => {
if ( value === maxPriceInput ) {
return;
Expand Down
35 changes: 35 additions & 0 deletions assets/js/base/components/price-slider/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/**
* External dependencies
*/
import { NumberFormatValues } from 'react-number-format';

export const withMaxValueLimit = ( {
maxConstraint,
minorUnit,
}: {
maxConstraint: number;
minorUnit: number;
} ) => ( { floatValue }: NumberFormatValues ): boolean => {
const maxPrice = maxConstraint / 10 ** minorUnit;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = ( {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Much better, would also add a comment to withMaxValueLimit.

minConstraint,
currentMaxValue,
minorUnit,
}: {
minConstraint: number;
currentMaxValue: number;
minorUnit: number;
} ) => ( { floatValue }: NumberFormatValues ): boolean => {
const minPrice = minConstraint / 10 ** minorUnit;
const currentMaxPrice = currentMaxValue / 10 ** minorUnit;

return (
floatValue !== undefined &&
floatValue >= minPrice &&
floatValue < currentMaxPrice
);
};