Add minmax filter #107
Add minmax filter #107
Conversation
|
Got another one for you @parksjr. No rush on it, won't need it in for a bit. |
|
@log0s some automated checks failed around testing |
Yeah I think that was just coverage again. Do you know how that's calculated? I added a basic test for the new component but maybe it wants more tests? |
|
Hey @log0s my bad for the delay, I did not see the reply notification. anyway, its jest test coverage and it scans files and functions within, detects covered and uncovered lines based on runnable tests, and it reports on each file in the logs (here is a link to the coverage report in the last run right here on this PR). And each file has percentages of line coverage, branch, and functions, and it also lists line numbers not covered. Here is what that looks like for the relevant components in this feature:
I think a lot of those lines in the new component could benefit from a little more test coverage. Since its a form component, add a user action test. Also, test out those min max error conditions. I hope that helps, I'll keep my eye on this and help you get it through when ready. |
|
No worries @parksjr, like I said no huge rush on this. Added a couple tests for the error display and that seems to have bumped it up enough. Let me know how it looks now. |
parksjr
left a comment
parksjr
left a comment
There was a problem hiding this comment.
Left some comments for you
| useEffect(() => { | ||
| if (maxError === true) { | ||
| handleInputChange({ target: { id: `${namePrefix}-max`, value: valueMaxLocal.toString() } }); | ||
| } | ||
| if (minError === true) { | ||
| handleInputChange({ target: { id: `${namePrefix}-min`, value: valueMinLocal.toString() } }); | ||
| } | ||
| }, [valueMinLocal, valueMaxLocal]); |
There was a problem hiding this comment.
Curious about this useEffect. It re-triggers the change handler when either of the error states is true. The handler itself is responsible for updating the error state. I removed it and things still work fine, but i might be missing something. Let me know what if there's something i'm missing about this so I can understand
There was a problem hiding this comment.
Not 100% sure, another thing I copied over and didn't quite understand completely. I'll take it out.
src/components/SearchFiltersMinMax/stories/SearchFiltersMinMax.Default.stories.js
Outdated
Show resolved
Hide resolved
src/components/SearchFiltersMinMax/stories/SearchFiltersMinMax.Default.stories.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Michael Parks <mike@parksjr.tech>
….Default.stories.js Co-authored-by: Michael Parks <mike@parksjr.tech>
|
Great feedback, thanks @parksjr. Let me know how it looks now. |
2 participants
Adds a new filter type and fixes a bug with the range filters.