-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DCJ-691] Add filter for primary data use #2687
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thank you 👍🏽. Separately, there had recently been a UX effort to move away from lodash. I'm neutral on the subject, so this works for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still new to this repo, but looks reasonable to me!
Re: @rushtong 's comment about lodash - I thought we were trying to move away from fp/lodash, but not regular lodash helpers. But, I'm not totally up to speed on that subject.
const isFiltered = (filter) => filters.indexOf(filter) > -1; | ||
|
||
const isFiltered = (filter, category) => (filters[category]).indexOf(filter) > -1; | ||
const numSelectedFilters = (filters) => Object.values(filters).reduce((sum, array) => sum + array.length, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with the lodash reduce function, would you mind explaining what is happening here? Why couldn't we just the length of the array of filters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can definitely do something like filters.accessManagement.length + filters.dataUse.length
, but this mechanism avoids having to remember to add new filters to this function. reduce
is a Javascript built-in, it runs a callback over each element of the array with an accumulator, with 0 (in this case) set as the initial value.
Object.values(filters)
transforms { f1: ['a', 'b', 'c'], f2: ['d', 'e', 'f'] }
into [ ['a', 'b', 'c'], ['d', 'e', 'f'] ]
then reduce
loops over each item in the array (which are subarrays), adds the length of the subarrays to the sum (which starts at zero), and so ultimately returns the total number of selected filters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE: lodash, that was also my understanding, moving specifically away from the fp
version of lodash, not the regular version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank for explaining that!
Addresses
https://broadworkbench.atlassian.net/browse/DCJ-691
Summary
Revamps the filtering process to be scalable for many different filters, and adds a filter for primary data use. In general, the filtering works by:
filterHandler
)assembleFullQuery
)search
)What was changed here is that previously the selections for the (one and only) filter were hardcoded and added to an array, and then the state and query were constructed from that array. Now instead:
filterHandler
also accepts a category of filter, and these are stored in an object that corresponds to the filter category and the selections.{dataset}.dataUse.primary.code
is a deeply nested JSON object which ElasticSearch has indexed a particular wayIn the future, the mechanism to add new filters should be straightforward: