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
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ interface FormattedMonetaryAmountProps
extends Omit< NumberFormatProps, 'onValueChange' > {
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

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.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { Currency, isObject } from '@woocommerce/types';
import './style.scss';
import { constrainRangeSliderValues } from './constrain-range-slider-values';
import FilterSubmitButton from '../filter-submit-button';
import { isValidMaxValue, isValidMinValue } from './utils';

export interface PriceSliderProps {
/**
Expand Down Expand Up @@ -240,9 +241,27 @@ const PriceSlider = ( {
) {
return;
}

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

// 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 @@ -351,6 +370,12 @@ const PriceSlider = ( {
'Filter products by minimum price',
'woo-gutenberg-products-block'
) }
allowNegative={ false }
isAllowed={ isValidMinValue( {
minConstraint,
minorUnit: currency.minorUnit,
currentMaxValue: maxPriceInput,
} ) }
onValueChange={ ( value ) => {
if ( value === minPriceInput ) {
return;
Expand All @@ -369,6 +394,10 @@ const PriceSlider = ( {
'Filter products by maximum price',
'woo-gutenberg-products-block'
) }
isAllowed={ isValidMaxValue( {
maxConstraint,
minorUnit: currency.minorUnit,
} ) }
onValueChange={ ( value ) => {
if ( value === maxPriceInput ) {
return;
Expand Down
41 changes: 41 additions & 0 deletions assets/js/base/components/price-slider/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* External dependencies
*/
import { NumberFormatValues } from 'react-number-format';

/**
Check if that the value is minor than the max price and greater than 0.
*/
export const isValidMaxValue = ( {
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 && floatValue > 0;
};

/**
Check if that the value is minor than the max price and greater than 0.
*/
export const isValidMinValue = ( {
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
);
};