Fixes and improvements for the Price Slider Block #2784
Conversation
mikejolley
added
status: needs review
block: filter by price
| @@ -60,6 +59,7 @@ const FormattedMonetaryAmount = ( { | |||
| displayType: 'text', | |||
| ...props, | |||
| ...currencyToNumberFormat( currency ), | |||
| fixedDecimalScale: Math.round( priceValue ) !== priceValue, | |||
There was a problem hiding this comment.
FYI this prevents .00 showing for prices in the slider.
There was a problem hiding this comment.
Won't this result in prices like 10.10 showing as 10.1 though? This component is used in a lot of different places, are we sure we don't want 10.00 showing in all cases where this component is used (as opposed to just the price slider)?
| @@ -257,18 +265,16 @@ const PriceSlider = ( { | |||
| 'Filter products by minimum price', | |||
| 'woo-gutenberg-products-block' | |||
| ) } | |||
| value={ | |||
There was a problem hiding this comment.
Removing value here and instead changing values dynamically allows keyboard input to function.
There was a problem hiding this comment.
This is a bit of an antipattern in React because you are changing things imperatively instead of declaratively (see "When to Use Refs"). In the case of inputs it's usually okay to use refs to get various attributes of the input when it changes but not to set values via the ref.
I understand that this might have appeared as a way to fix allowing the keyboard input to function but are we sure this is the only way to fix that? If necessary, to avoid blocking the other fixes from getting released - addressing that specific issue could be split out separately from this pull so we avoid implementing this anti-pattern.
I notice through this review that we're mutating other ref properties here for the input as well (zIndex on style) that should be done through react props instead of through the ref (although I realize they were pre-existing this pull...they still should be addressed).
|
Size Change: +5.29 kB (0%) Total Size: 1.64 MB
ℹ️ View Unchanged
|
| @@ -60,6 +59,7 @@ const FormattedMonetaryAmount = ( { | |||
| displayType: 'text', | |||
| ...props, | |||
| ...currencyToNumberFormat( currency ), | |||
| fixedDecimalScale: Math.round( priceValue ) !== priceValue, | |||
There was a problem hiding this comment.
Won't this result in prices like 10.10 showing as 10.1 though? This component is used in a lot of different places, are we sure we don't want 10.00 showing in all cases where this component is used (as opposed to just the price slider)?
| const hasMinConstraint = Number.isFinite( minAllowed ); | ||
| const hasMaxConstraint = Number.isFinite( maxAllowed ); | ||
| const minConstraint = minAllowed || 0; | ||
| const maxConstraint = maxAllowed || step; |
There was a problem hiding this comment.
Should there be extra validation on the step value as well?
| let minValue = parseValid( values[ 0 ], minConstraint ); | ||
| let maxValue = parseValid( values[ 1 ], maxConstraint ); |
There was a problem hiding this comment.
So if values[0] or values[1] are not "valid" the fallback is minConstraint or maxConstraint (which in turn might be 0 or step). Should we have some additional validation on minConstraint/maxConstraint at this point to ensure they are valid values for comparing with (if they end up used as fallbacks) later on in the function? The original values are validated at the beginning of the function to determine hasMinConstraint and hasMaxConstraint values but that doesn't allow for the potential that they could be strings or some other non-truthy, non numeric value when used in these expressions as fallbacks?
(I'm not sure how far we should go with being defensive here, so any pushback with rationale you have on this is welcome)
|
|
||
| const [ minPriceInput, setMinPriceInput ] = useState( minPrice ); | ||
| const [ maxPriceInput, setMaxPriceInput ] = useState( maxPrice ); | ||
|
|
||
| useEffect( () => { | ||
| if ( minRange.current ) { | ||
| minRange.current.value = minPrice; |
There was a problem hiding this comment.
I realize the value property on the ref comes from passing this through as a ref on the input. However, the ref is initialized as minRange.current = null. Currently, the code works because it just so happens when minRange.current is truthy, it's because it has been converted to an object but it is still fragile because it's implied via passing through as a ref on input (which isn't immediately apparent).
I think it might be better here if you explicitly initialize the ref with the expected data structure, so something like:
const minRange = useRef( {} );
Then you can modify your conditionals to be:
if ( minRange.current.value !== undefined ) { /* ... */ }
| const minX = minWidth * ( minValue / maxConstraint ); | ||
| const maxX = maxWidth * ( maxValue / maxConstraint ); |
There was a problem hiding this comment.
minWidth or maxWidth might be undefined here right? So this could result in NaN. Have you accounted for that in the various subsequent comparisons derived from these values?
| currentMinRange.style.zIndex = 20; | ||
| currentMaxRange.style.zIndex = 21; | ||
| } else { | ||
| minRange.current.style.zIndex = 21; | ||
| maxRange.current.style.zIndex = 20; | ||
| currentMinRange.style.zIndex = 21; | ||
| currentMaxRange.style.zIndex = 20; |
There was a problem hiding this comment.
There's an assumption here that style exists as a property on the range values. There's risk here of getting assignment warnings/error for zIndex.
| @@ -257,18 +265,16 @@ const PriceSlider = ( { | |||
| 'Filter products by minimum price', | |||
| 'woo-gutenberg-products-block' | |||
| ) } | |||
| value={ | |||
There was a problem hiding this comment.
This is a bit of an antipattern in React because you are changing things imperatively instead of declaratively (see "When to Use Refs"). In the case of inputs it's usually okay to use refs to get various attributes of the input when it changes but not to set values via the ref.
I understand that this might have appeared as a way to fix allowing the keyboard input to function but are we sure this is the only way to fix that? If necessary, to avoid blocking the other fixes from getting released - addressing that specific issue could be split out separately from this pull so we avoid implementing this anti-pattern.
I notice through this review that we're mutating other ref properties here for the input as well (zIndex on style) that should be done through react props instead of through the ref (although I realize they were pre-existing this pull...they still should be addressed).
| if ( ! Number.isFinite( value ) ) { | ||
| value = 0; | ||
| } |
There was a problem hiding this comment.
Should this come before the comparison with minPriceInput (same question applies for the maxPriceInput comparison a bit later)?
| const formattedPrice = renderToString( | ||
| <FormattedMonetaryAmount | ||
| currency={ currency } | ||
| displayType="text" | ||
| value={ priceInt } | ||
| /> | ||
| ); | ||
|
|
||
| // This uses a textarea to magically decode HTML currency symbols. | ||
| const txt = document.createElement( 'textarea' ); | ||
| txt.innerHTML = formattedValue; | ||
| return txt.value; | ||
| // Remove HTML. | ||
| const tagsRegExp = /<\/?[a-z][^>]*?>/gi; | ||
| return formattedPrice.replace( tagsRegExp, '' ); |
There was a problem hiding this comment.
Hmm... interesting approach here. I originally did a double-take because I thought renderToString could only be used reliably in the server context but it looks like it can be used in both environments now.
It'd be good to add to the tests some cases around removing html.
| export const formatPrice = ( price, currencyData ) => { | ||
| export const formatPriceAsString = ( price, currencyData = {} ) => { |
senadir
force-pushed
the
update/price-slider-fixes
branch
from
c9c8249 to
0dd3d2d
Compare
|
I assigned this PR to me, will tackle it somewhere next week and address Darren's review. |
|
Unassigning myself from this because I don't intend on working on it anytime soon, and removing the review request so it doesn't influence the load balance algorithm. |
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label - otherwise it will automatically be closed after 10 days. |
github-actions
bot
added
the
status: stale
nerrad
added
type: cooldown
|
@ralucaStan I forgot this existed. This touches on some of the fixes you've been working on. This is out of sync so probably better to start fresh anyway. |
|
I'm going to bin this PR. It's stale and all the issues could use a fresh pair of eyes. I see @ralucaStan is assigned to some of the original issues now. |
Resolves some related issues reported with the Price Slider. Tackled in one pull due to touching the same code for each.
minorUnitvalue #1657How to test the changes in this Pull Request:
Add All Products, Price Filter, and Active Filters to a page.
Changelog