refactor: InventoryTable #2129
refactor: InventoryTable #2129
Conversation
|
@bastilian It seems it is better to be updated with the master before I review it. Because, there maybe a lot of changes that affect the behavior. |
| * to get the latest props and not the props at the time of when the function is | ||
| * being wrapped in callback. | ||
| */ | ||
| export const inventoryCache = () => { |
There was a problem hiding this comment.
The inventoryCache seems unnecessary, since we hold these params and props in many places already.
| @@ -3,13 +3,7 @@ import PropTypes from 'prop-types'; | |||
| import { usePermissionsWithContext } from '@redhat-cloud-services/frontend-components-utilities/RBACHook'; | |||
| import { GENERAL_HOSTS_READ_PERMISSIONS } from '../constants'; | |||
|
|
|||
| const RenderWrapper = ({ | |||
| cmp: Component, | |||
| isRbacEnabled, | |||
There was a problem hiding this comment.
This prop is not really needed or used (please double check)
| @@ -46,7 +44,6 @@ export const AccountStatContext = createContext({ | |||
| }); | |||
|
|
|||
| export const Routes = () => { | |||
| const searchParams = useMemo(() => getSearchParams(), []); | |||
There was a problem hiding this comment.
We shouldn't pass this in here. all components that use params lower in the tree should access theme vie the react router (params) hook
| /** | ||
| * Component that works as a side channel for consumers to notify inventory of new data changes. | ||
| */ | ||
| const ContextInventoryList = ({ |
There was a problem hiding this comment.
This is a redundant wrapper. Removing it will simplify the structure and should reduce the render burden.
| /** | ||
| * Debounced `updateData` function. | ||
| */ | ||
| const debouncedRefresh = useCallback( |
There was a problem hiding this comment.
This is mainly the culprit as far as I remember, starting from line 247.
These functions and callbacks were called when they shouldn't have been, due to the debouncing here. These changes in the table and the toolbar change that a little and consolidate everything to only call the ONE debounced refresh function in the EntityTable above.
| } | ||
| } | ||
| }; | ||
|
|
||
| const prevFilters = useRef(customFilters); | ||
| const debouncedRefresh = debounce((config) => onRefreshData(config), 800); |
There was a problem hiding this comment.
this is the ONE debounced refresh function that should be called. And the only way that any refresh should be triggered.
| /> | ||
| <Grid gutter="sm" className="ins-inventory-list"> | ||
| <GridItem span={12}> | ||
| <InventoryEntityTable {...props} onRefreshData={onRefreshData} /> |
There was a problem hiding this comment.
It seems that removing the redundant wrapper out of here has broken the fetch when sorting has changed. @bastilian was your intention to pass onRefreshData to EntityTable and refresh the data when sorting changes, from within the component?
There was a problem hiding this comment.
@johnsonm325 Do you have the current changes somewhere I can take a look at them?
No description provided.