#756 User is able to select filters quickly #799
Conversation
There was a problem hiding this comment.
Looks great @Shaglock . Apologies for the long delay in reviewing this!
Some thoughts below.
On testing
TBH, i'm not convinced about the testing strategy that was in place here. IIUC, these tests are checking what gets passed into an event handler when a filter is clicked. IMO this only tests a slice of the functionality, ie that we wired the click handler correctly in vue. Also feels like testing implementation details instead of outcomes.
It could be better to mount the component with some known data, click and then check whether the expected contributions are shown. But maybe i'm misreading how these work?
In any case, i realize you are following the precedent set in the example above, and what i'm suggesting is considerably more work to set up!
Tagging @h-m-m in case he has opinions on this.
FilterType component?
Looking over the code, i'm noticing that filters gets passed into three of the new methods and currentFiltersForFilterType gets used a lot. I wonder if this might be hinting at the possibility of extracting a FilterType or similar sub-component?
Each instance of this sub-component would handle only the filters for a single filter type, eliminating the need to pluck out relevant filters in several places.
Just a thought; i'm not sure without trying whether this would be an overall simplification. For one thing, Filters would need to change somewhat to aggregate the current filters from each of the FilterType instances.
Merge?
All that said, i'm happy to merge as is since this is a huge improvement. We could add a separate issue to revisit the specs and any refactorings you/we could consider. LMK if you want to modify anything before i merge.
Thank you! 🙌🏾 🚀
| @@ -59,6 +67,16 @@ export default { | |||
| prop: 'currentFilters', | |||
| event: 'change', | |||
| }, | |||
| computed: { | |||
| allFiltersSelected: function () { | |||
| return JSON.stringify(this.allFilters.sort()) == JSON.stringify(this.currentFilters.sort()) | |||
There was a problem hiding this comment.
Minor (mostly curious): would this.allFilters.size === this.currentFilters.size also work?
There was a problem hiding this comment.
yes, it works. I don't remember why I did it in such a strange way :D thank you for reviewing
Shaglock
force-pushed
the
quick-filters
branch
from
5a08e89
to
5b33a67
Compare
|
Hello @exbinary, thank you very much for your review! I suggest to merge this PR and add separate issue(s) to revisit the specs. |
Why
Resolves #756.
Pre-Merge Checklist
What
I added the ability to check all/uncheck all filters by column as well as the entire filters section. If any of the filters in the column is checked, the checkbox for select all shows an indeterminate state.
How
I guess the code is pretty self-explanatory, I tried to keep it simple. I added toggle functions and some helpers to detect the state of the checkboxes.
Testing
I changed the behavior of the old test a little bit because there are more checkboxes now. I am not a frontend developer, this is my first time writing tests for Vue, I tried to follow the example of an existing test. I guess the next step is to add more tests for the functionality I created, for example, test indeterminate state of checkboxes, test for toggle functions. I think the tests I added are not good enough and I am not sure I can do better 😄 But I manually tested everything and it works well.
Next Steps
Outstanding Questions, Concerns and Other Notes
Accessibility
Security
Meta