New block: Filter Products by Price #853
Conversation
mikejolley
added
status: in progress
type: new block
|
@nerrad ready for review :) |
There was a problem hiding this comment.
This is really good progress @mikejolley 🎉 👍 ! I'm still reviewing but thought I'd submit what I have so far in terms of feedback.
- why is
assets/js/base/components/price-slider/handles.sketchin here? Should be removed? - I'm seeing styling issues with the slider in both the editor:
...and the frontend:
Both the above are with the storefront theme (in Chrome browser).
- It is general good practice in react to have all input type components be controlled vs uncontrolled in react. I realize the PriceFilter control is a bit on the border of being an "input" being it's more composite. I did a bit of exploration and I think the main hangup preventing trivially switching this component to be controlled vs uncontrolled is the "Go" button option as in order to trigger the onChange event it needs to know what value to give the callback (and it requires context on whether the last user interaction was on the min slider/input or the max slider/input). To resolve that would require some sort of local state and in that case it wouldn't vary too much from what you have now. So... long story short, let's leave this as uncontrolled for now 👍 .
- all the
render*callbacks you have could be extracted to be actual functional components and put them in their own files only used internally byPriceSlider. That keeps thePriceSlidercontents more readable and easier to follow.
Re naming
I have some suggestions for naming variables etc in here, however is this a component that could potentially be used for more than just price (It looks like it might). If so, then it might be good to actually genericize the naming a bit more. So instead of this being a PriceSlider component, it could just be something like ComplexRangeSlider and that would allow for greater re-usability for other types of values that are controlled this way? If you agree, then you can ignore some of my suggestions about including price in other variable names.
To make more generic, you'd probably also want to provide a formatter callback to component and that can be used for doing any necessary formats for displaying the input values of the slider.
| ); | ||
|
|
||
| if ( containers.length ) { | ||
| Array.prototype.forEach.call( containers, ( el ) => { |
There was a problem hiding this comment.
You could just do containers.forEach here?
There was a problem hiding this comment.
This is needed for ie11 compatibility, since we're not using lodash helpers.
There was a problem hiding this comment.
Existing pattern in existing code FYI
|
I think this is ready for another look @nerrad |
There was a problem hiding this comment.
Coming along great! Thanks for implementing the changes from the feedback. There's still a few things I think need addressed. Also, we'll probably want to exclude this block from legacy builds as well, so it can happen similarly as the all-products block:
- configure
webpack.config.jsso that legacy configs exclude the path for this block. - change
Library.phpso price filter is moved to the same array as the all products block.
| ), | ||
| } ); | ||
| }, [ min, max ] ); | ||
| }, [ minConstraint, maxConstraint ] ); |
There was a problem hiding this comment.
minPrice, and maxPrice should be dependencies as well otherwise this check won't run if they change.
There was a problem hiding this comment.
Doesn't need to; this check only needs to run if constraints change. If the min/max current changes it doesn't matter to this block - its within the constraints already.
There was a problem hiding this comment.
Hmm, what about changes in the text input, or do we care about that?
package-lock.json
Outdated
| "integrity": "sha512-Wt2sNMKAX/CKeeeQkdPtolTDcP1nPjvPnXiKLL116h89i4nJ3gBgS6PA31XEeDFuj/tq8HiYeMbzfijKhJyHsA==", | ||
| "version": "1.3.3", | ||
| "resolved": "https://registry.npmjs.org/@wordpress/data-controls/-/data-controls-1.3.3.tgz", | ||
| "integrity": "sha512-MA67FZi733IAfUGSqjt/J1yGyls4xgnl6Z+l57ihpEcB6QCxRhWS4Nl3nOuAkzgtPvPgbTQTns/c3dTDpda2uw==", |
There was a problem hiding this comment.
🤔 huh! I was wondering about this but I guess wp rolled back the patch versions (hence the changes in package-lock.json here for the @wordpress/* packages.)
|
Should be resolved now. |
There was a problem hiding this comment.
Looks great! Are we going to iterate on any necessary styling improvements in separate pulls? Also I noticed some quirkiness in results returned from some queries (but I think that might be a result of the endpoint vs anything in the component - might change with switching to the new store route too).
|
Styling improvements of the all products block as a hole should be done during QA and in a separate pull when we're done, yes. Thanks for the review. |
Introduces the filter products by price block (#710).
Screenshots
How to test the changes in this Pull Request:
Changelog