Feature/772 search #802
Feature/772 search #802
Conversation
Implement search by plan title by filtering case triggered plans and routine plans that match the search parameter
Delay search to make sure the filter is not applied to frequently
kelvin-muchiri
requested review from
moshthepitt,
cKellyDesign,
kahummer,
ciremusyoka and
peterMuriuki
|
Question: why not do this using a reselect selector at the store level? It should be possible to make that re-useable as well, right? I'm leaning towards that because:
Let me know what you think. |
|
@moshthepitt Let me look into the libabry never worked with it before. It may be of help |
|
@moshthepitt I was able to use reselect for |
|
@moshthepitt Shouldn't this part use selectors? |
@kelvin-muchiri that's correct. The context is that reselect is being added slowly. Whatever needs https://github.com/onaio/reveal-frontend/blob/master/src/store/ducks/plans.ts#L294 can probably be refactored to use https://github.com/onaio/reveal-frontend/blob/master/src/store/ducks/plans.ts#L523. |
src/containers/pages/InterventionPlan/PlanDefinitionList/index.tsx
Outdated
Show resolved
Hide resolved
| export interface SearchFormProps { | ||
| history: History; | ||
| location: Location; | ||
| placeholder?: string; |
There was a problem hiding this comment.
Can we make this not optional? It seems we already have a desired default, which can be supplied to this component via defaultProps. Parse, don't validate.
There was a problem hiding this comment.
@kelvin-muchiri can we make this not optional? i.e. remove the ?
src/containers/pages/InterventionPlan/PlanDefinitionList/index.tsx
Outdated
Show resolved
Hide resolved
...containers/pages/InterventionPlan/PlanDefinitionList/tests/__snapshots__/index.test.tsx.snap
Show resolved
Hide resolved
There was a problem hiding this comment.
@kelvin-muchiri great work man, this looks good to me once the last few comments from @p-netm and @moshthepitt are addressed (and tickets are generated!)
…mponent IRSPlansList
Use selector makePlanDefinitionsArraySelector in place of getPlansArrayByTitle selector. Add tests for selector makeIRSPlansArraySelector
@moshthepitt What issue is this? |
@kelvin-muchiri the fact that when we get an empty list of plans we will continue showing a Loading indicator |
src/helpers/utils.tsx
Outdated
|
|
||
| /** | ||
| * Get query params from URL | ||
| * @param {Location} location location object from props |
There was a problem hiding this comment.
Seems this is repeated location location
| } | ||
|
|
||
| /** plansArrayBaseSelector select an array of all plans | ||
| * @param state - the redux store | ||
| */ | ||
| export const plansArrayBaseSelector = (planKey?: string) => (state: Registry): Plan[] => | ||
| export const plansArrayBaseSelector = (planKey?: string) => (state: Partial<Store>): Plan[] => |
There was a problem hiding this comment.
@kelvin-muchiri what led to use changing from Registry to Partial<Store> in this file?
There was a problem hiding this comment.
@moshthepitt state from mapStateToProps is not of type Registry and will raise lint warning if passed as an argument. Or which is the best way to solve this?
|
This has passed QA. Please note that only the following search functionalities have been tested:
|
Closes #772
Added a reusable search form that is used for search. The search is done by calling
Array.filteron the list received from props and is accompanied by a delayed search (debounce) of 1s to make sure the filtering is not done too frequently. The search filter is currently by name/title only