LF-4138 Convert current filter implementation to TypeScript #3145
Conversation
Accidentally pulled an older version in
kathyavini
requested review from
antsgar and
Duncan-Brain
and removed request for
a team
| @@ -6,6 +6,24 @@ import { Underlined } from '../Typography'; | |||
| import { useTranslation } from 'react-i18next'; | |||
| import Button from '../Form/Button'; | |||
| import FilterGroup from '../Filter/FilterGroup'; | |||
There was a problem hiding this comment.
The data flow for the non-finance filters is:
- Filter Container (Filter/Task, Filter/CropVariety, Filter/Documents etc.)
- (This component) PureFilterPage
- FilterGroup + subfilters
- The container defines the filterRef and the relationship with the Redux store, and transforms the Redux formatted filters into the component-formatted filters used downstream.
- The PureFilterPage component is a layout component with buttons that control the overall clearing and applying (saving) of filters.
- The FilterGroup component displays each individual filter. Each controls their own update of the filterRef, and, through the passed onChange, dirties the parent filter state to allowing saving.
- The onApply handler passed from the container to the PureFilterPage saves the updated filterRef to the Redux store.
| @@ -1,5 +1,5 @@ | |||
| /* | |||
There was a problem hiding this comment.
The flow for transactions filter is
- Finances/Report -AND- Finances/TransactionFilter
- (This component) TransactionsFilterContent
- FilterGroup + subfilters
Dataflow differs in that:
- Filters are only coupled bi-directionally to the Redux state in Finances/TransactionFilter. The report filters set local component state (via their passed onChange) and have a read-only relationship with the Redux store
- The modal layout and calling of the onApply handler (done by the middle component in the original flow) are defined in the container instead
- The Redux filter state shape is transformed into the component-used shape here in a common component (this is a good pattern I think!)
- The Report Filter state is controlled via the passed onChange, which required modifying the callback passed from FilterGroup to the individual filters in LF-3622 finances dashboard report #2942.
- This is a very clever approach but was hard to type 😄
- It means that depending on the calling container, the parameter passed into the component's onChange is either completely ignored, or is used to set state which is passed to the API and needs to be a particular shape. It happens to be the wrong shape in the MultiSelect as written, but that combination of container + component is not yet seen in app (it will be)
There was a problem hiding this comment.
Thank you for these notes, very helpful! I think the structure itself is not entirely different (although the file names change), in the sense that the TransactionFilter container defines the ref and relationship with the Redux store, similarly to what the other filter containers do, and the TransactionsFilterContent component replaces FilterPage in defining the layout and calling FilterGroup. FilterPage could not be reused because in Finances (and in general from now on) we're not doing separate pages for filters, so I think the Finances pattern will likely need to be repeated in that sense for Animals too. The difference with Animals I think will be that we'll need to filter from the filters modal itself and from the KPIs, but both will need bidirectional flow to the Redux store. The KPIs, unlike the Finances report, will need to dispatch the actual action to Redux to modify what's being displayed. If we do at any point need a report for Animals too though, we'd be well setup to implement the same pattern as here in Finances.
Re this bit
It happens to be the wrong shape in the MultiSelect as written, but that combination of container + component is not yet seen in app (it will be)
I think I knew this at the time and chose to ignore any other filter types since like you mention they weren't used in Finances or any similar combination that required this onChange function to work properly. Will need to be adjusted, although that entire MultiSelect component would now be replaced by the new one anyway.
There was a problem hiding this comment.
TransactionsFilterContent component replaces FilterPage in defining the layout and calling FilterGroup
Ah sorry, I should have been more clear -- by layout I meant not of the filter components, but of the overall structure (specifically the title, apply button and the state that determines when that button can be clicked), which in finances were lifted up one level to the container and in the others, sent down one level to the Page.
FilterPage could not be reused because in Finances (and in general from now on) we're not doing separate pages for filters
Okay now I'm realizing the two situations are even more confusing than I initially thought because... yes, this is exactly right! Aaaaand... all those other filters ARE already modals, not pages! Complete with Drawer behaviour. And they do it by situating the filter container within a Drawer, to which they don't pass buttons, because they let that interior page handle the buttons 😅
So I think both patterns are still technically viable, but I like the non-nesting approach better for sure; <Layout /> doesn't look that attractive within a <Drawer /> and there is less flexibility to style.
| // setDirty: () => !isDirty && setIsDirty(true) | ||
| export type FilterPageOnChangeCallback = () => void; | ||
|
|
||
| // type ContainerOnChangeCallback = FinanceReportOnChangeCallback | FilterPageOnChangeCallback; |
There was a problem hiding this comment.
I would love to define this as a union, but then the types shown on hover are the names of the union elements and not the full type.
Resetters are no longer defined or used anywhere in the application other than as an optional prop in PureFilterPage, so must have been from a previous implementation.
| const FilterItem = ({ filter, showIndividualFilterControls, ...props }) => { | ||
| if ((filter.type === PILL_SELECT || !filter.type) && filter.options.length > 0) { | ||
| // i.e (filterState) => ContainerOnChangeCallback(filter.filterKey, filterState) | ||
| export type ComponentOnChangeCallback = (filterState: FilterState) => void; |
There was a problem hiding this comment.
I think a more common pattern here might be to export the whole props interface, as in "I know this component will be reused by others and those others may need to know the shape it receives", rather than just exporting a specific property. I'd probably do
export interface FilterItemProps {
filter: ComponentFilter;
filterRef: React.RefObject<ReduxFilterEntity>;
onChange: ComponentOnChangeCallback;
shouldReset?: number;
showIndividualFilterControls?: boolean;
}
and then in the other component
onChange: FilterItemProps['onChange']
What you did here isn't wrong though! That'd just be my personal preference and would scale a bit better IMO if the component was reused somewhere else that needed access to other props
| filter: ComponentFilter; | ||
| filterRef: React.RefObject<ReduxFilterEntity>; | ||
| onChange: ComponentOnChangeCallback; | ||
| shouldReset?: number; |
There was a problem hiding this comment.
Not related to this PR at all but I find it very unintuitive that shouldReset is a number and not a boolean 😅
There was a problem hiding this comment.
I know; agreed 😭
| export type ContainerOnChangeCallback = | ||
| | ((filterKey: string, filterState: FilterState) => void) // Finance Report | ||
| | (() => void); // PureFilterPage and TransactionFilter |
There was a problem hiding this comment.
I would suggest defining this more generically as
export type ContainerOnChangeCallback = (filterKey?: string, filterState?: FilterState) => void
There needs to be some uniformity to the callback's shape since the FilterGroup component will always be calling it with the same parameters, the only difference is whether the container cares about them or not
There was a problem hiding this comment.
I think my main goal -- because it was the hardest thing for me to understand reading these files -- was trying to convey that in some cases the component onChange is just calling setDirty and that passed state argument -- which particularly reading the component + FilterGroup as a pair would make me assume it's always doing something significant -- is completely dropped; completely irrelevant.
But maybe my original types didn't do that very clearly either... or maybe it's not even something that can/should be expressed in Types since it's not exactly about the function signature? Or do the optional parameters as above makes it clear enough? 🤔 I'm not really sure...!
(Edit: Change has been made as above though)
There was a problem hiding this comment.
I see your point! But we could probably throw in a comment instead to clarify that
|
|
||
| // In tempStateReducer > filterReducer | ||
| export interface ReduxFilterEntity { | ||
| [filterKey: /* e.g. STATUS, LOCATION */ string]: FilterState; |
There was a problem hiding this comment.
It might be nice for the type for filterKey to be passed as an optional generic so that if we need to call this type from more specific filter components we can specify the enum
export type ReduxFilterEntity<FilterKey extends string = string> = Record<FilterKey, FilterState>;There was a problem hiding this comment.
I can't read generics yet, but I'll give this a close study, thank you 😄
There was a problem hiding this comment.
Ah okay, so we can call it like
animalsFilter: ReduxFilterEntity<AnimalsFilterKeys>Nice! 👍
| @@ -38,7 +49,7 @@ export const FilterMultiSelect = ({ | |||
| }, [shouldReset]); | |||
|
|
|||
| useEffect(() => { | |||
| filterRef.current[filterKey] = produce(defaultFilterState, (defaultFilterState) => { | |||
| filterRef.current![filterKey] = produce(defaultFilterState, (defaultFilterState) => { | |||
There was a problem hiding this comment.
Does it break if you wrap this in if(filterRef.current) {}?
This could hide errors otherwise
There was a problem hiding this comment.
I doubt it would break, but I wanted to avoid changing the JS logic the way a new conditional would (while the non-null assertion is just for the type checker). Since the code works, I'm assuming we don't need the check for functionality? Sounds like @antsgar will be re-writing this one though anyway.
There was a problem hiding this comment.
Without knowing anything -- even though the code works there might just be an edge case we haven't reached yet but its possible the type checker missed a conditional somewhere else. 🤷
There was a problem hiding this comment.
The filterRef current value could technically be null so it'd be correct to add the conditional here I think, but this component will all get re-written anyway!
| export type FinanceReportOnChangeCallback = (filterKey: string, filterState: FilterState) => void; | ||
|
|
||
| // setDirty: () => !isDirty && setIsDirty(true) | ||
| export type FilterPageOnChangeCallback = () => void; |
There was a problem hiding this comment.
Are you keeping this one?
There was a problem hiding this comment.
No, I'll do as @antsgar suggested above, but I'm still looking for the 100% pleasing solution to convey that sometimes this is just setDirty()! Any thoughts? Does the chain of onChange read okay to you overall and in the component file (as a previous consumer of the filter pattern when building the sales hook)?
I know I didn't actually type the component you used (since we're not touching it anymore this sprint) so maybe the filter container behaviour is less relevant.
There was a problem hiding this comment.
I doooon't remember lol I just remember thinking it all seems too complex for what it needs to do. As for hovering to see types I am comfortable right clicking to follow to source or typing the subtype.
| style={style} | ||
| placeholder={`${t('common:SELECT')}...`} | ||
| //@ts-ignore | ||
| style={style} // I suspect the forwardRef is at fault for the type error? |
There was a problem hiding this comment.
May as well type that one out too? lol
https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/forward_and_create_ref/
There was a problem hiding this comment.
Noooooo not the React Select 🤣 I really didn't want to touch that, but I guess it's imported a lot so is worth fixing; Nav had issues on his component too. Thanks for link.
Description
This was originally intended to assist with the new filter component; however, I'm hoping it will be equally if not more useful in terms of understanding the filter dataflow from Redux to the component for use in Animals.
Any feedback on readability (particularly on the placement, naming, and structure of interfaces -- which was the main point of doing this conversion -- would be so appreciated!! 🙏
Notes:
SEARCHABLE_FILTER_MULTI_SELECTwas not, in fact, searchable because it was missing itsisSearchableprop... that has been added (or more likely, restored).Jira link: https://lite-farm.atlassian.net/browse/LF-4138
Type of change
The placeholder and the searchability of the multi-select filter have been fixed; otherwise all changes should be runtime silent as they are just in the typescript types.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist:
Note: I would have liked to add licenses to some of the files I touched, but GitHub's ability to detect a JSX -> TSX conversion as the same file (vs a delete + new file) is brittle, and seems to require changing as few lines of code as possible.