-
Notifications
You must be signed in to change notification settings - Fork 4
persist computation result filters #3484
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
base: main
Are you sure you want to change the base?
persist computation result filters #3484
Conversation
…ters-persist-state' into implement-computation-result-filters-persist-state
|
flomillot
left a comment
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.
Some renaming to do to be more acurate and understable
- onFilter -> onFilterChange
- api -> agGridApi
|
|
||
| const { countriesFilter, voltageLevelsFilter, propertiesFilter } = useGlobalFilterOptions(); | ||
| const { globalFilters, handleGlobalFilterChange } = useGlobalFilters(); | ||
| const { handleGlobalFilterChange, globalFilters } = useGlobalFilters(); |
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.
the setter at the end, keep original order?
|
|
||
| const { persistFilters } = useComputationColumnsFilters(AgGridFilterType.Loadflow, mappingTabs(tabIndex)); | ||
|
|
||
| const onFilter = useCallback( |
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.
| const onFilter = useCallback( | |
| const onFilterChange = useCallback( |
| [dispatchGlobalFilters, studyUuid, config?.id] | ||
| ); | ||
|
|
||
| return useMemo( |
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 don't understand why this useMemo ?
| [studyUuid, computationResultFilterUuid, columnUuid] | ||
| ); | ||
|
|
||
| return useMemo( |
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.
Same
| ); | ||
|
|
||
| const persistFilters = useCallback( | ||
| (colId: string, api: GridApi, filters: FilterConfig[]) => { |
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.
| (colId: string, api: GridApi, filters: FilterConfig[]) => { | |
| (colId: string, agGridApi: GridApi, filters: FilterConfig[]) => { |
| useEffect(() => { | ||
| // Clear the globalfilter when tab changes | ||
| handleGlobalFilterChange([]); | ||
| }, [handleGlobalFilterChange]); | ||
|
|
||
| return ( |
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.
revert this change because you just moved the use effect so we can keep the git history
|
|
||
| const { persistFilters } = useComputationColumnsFilters(FilterType.PccMin, PCCMIN_RESULT); | ||
|
|
||
| const onFilter = useCallback( |
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.
same
| ); | ||
|
|
||
| const onFilter = useCallback(() => { | ||
| const memoizedSetPageCallback = useCallback(() => { |
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 know it's not originally from you but I don't understand the name of this callback
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.
why it's not goFirstPage or something like this ?
| dataType: c.filterDataType, | ||
| tolerance: c.filterTolerance ?? undefined, | ||
| }); | ||
| export function setComputationResultFiltersState(filtersInfos: ComputationResultFiltersInfos): ComputationFiltersState { |
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.
| export function setComputationResultFiltersState(filtersInfos: ComputationResultFiltersInfos): ComputationFiltersState { | |
| export function getComputationResultFiltersState(filtersInfos: ComputationResultFiltersInfos): ComputationFiltersState { |
or create / init but not set
| resetTableDefinitions(collection); | ||
| }); | ||
|
|
||
| const fetchComputationResultFiltersPromise = getComputationResultFilters(studyUuid).then( |
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.
| const fetchComputationResultFiltersPromise = getComputationResultFilters(studyUuid).then( | |
| const fetchComputationResultFilters = getComputationResultFilters(studyUuid).then( |
no need to add promise when you already have fetch in the name function
| import { updateFilters } from '../components/custom-aggrid/custom-aggrid-filters/utils/aggrid-filters-utils'; | ||
| import { GridApi } from 'ag-grid-community'; | ||
|
|
||
| export function useComputationColumnsFilters(filterType: FilterType, filterTab: string) { |
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.
| export function useComputationColumnsFilters(filterType: FilterType, filterTab: string) { | |
| export function useUpdateComputationColumnsFilters(filterType: FilterType, filterTab: string) { |
as the only thing you return is persistFilters which update filters
| (state: any) => state.computationFilters?.[filterType]?.columnsFilters?.[filterTab]?.id | ||
| ); | ||
|
|
||
| const persistFilters = useCallback( |
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.
| const persistFilters = useCallback( | |
| const updateColumnFilters = useCallback( |
| import { updateFilters } from '../components/custom-aggrid/custom-aggrid-filters/utils/aggrid-filters-utils'; | ||
| import { GridApi } from 'ag-grid-community'; | ||
|
|
||
| export function useComputationColumnsFilters(filterType: FilterType, filterTab: string) { |
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.
Maybe not here If we are short in time, so maybe in the next pull request :
- rename the hook into
useColumnsFilters - include
const { filters } = useFilterSelectorand returnfilterslike for global filters - remove
const { filters } = useFilterSelectorfrom other components
| // @ts-expect-error TODO: found a better way to go into state | ||
| (state: AppState) => state[FILTER_PARAMS[filterType].filterType][filterTab] | ||
| ); | ||
| const entry = FILTER_PARAMS[filterType]; |
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.
Is it possible to rename the variable and add a comment to explain the code ? personally I don't understand this part
| }; | ||
| }); | ||
| }); | ||
| builder.addCase(UPDATE_COLUMN_FILTERS, (state, action: UpdateColumnFiltersAction) => { |
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.
Much better indeed 👍 nice
| subtypeSubstationProperties.push(filter.label); | ||
| } else { | ||
| substationProperties.set(filter.filterSubtype, [filter.label]); | ||
| value.forEach((filter) => { |
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.
👍
| FilterType as AgGridFilterType, | ||
| textFilterParams, | ||
| numericFilterParams, | ||
| textFilterParams, |
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.
revert for history
| tab: mappingTabs(tabIndex), | ||
| updateFilterCallback: onFilter, | ||
| }; | ||
| const createUpdateFilterCallback = (colId: string) => (api?: GridApi, filters?: FilterConfig[]) => { |
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.
Duplicated as say sonar.
| filterValue: filter?.value, | ||
| filterTolerance: filter?.tolerance, | ||
| }; | ||
| updateFilters(api, 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.
I don't understand why it's here and not directly inside useCustomAggridFilter ?
| import { updateFilters } from '../components/custom-aggrid/custom-aggrid-filters/utils/aggrid-filters-utils'; | ||
| import { GridApi } from 'ag-grid-community'; | ||
|
|
||
| export function useComputationColumnsFilters(filterType: FilterType, filterTab: string) { |
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.
Actually, did you really need this hook?
You could not put this code inside useCustomAggridFilter?


PR Summary
Manage all the calculation result filters with a single hook.