-
Notifications
You must be signed in to change notification settings - Fork 17
fix: format bytes columns and avoid refetch on column toggle #3149
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?
Changes from 1 commit
50bad3d
1a28b1d
d837d77
61c41df
7b28581
f29b457
a8deff0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,7 @@ interface TableChunkProps<T, F> { | |
| onDataFetched: (data?: PaginatedTableData<T>) => void; | ||
|
|
||
| keepCache?: boolean; | ||
| useColumnsIdsInRequest?: boolean; | ||
| } | ||
|
|
||
| // Memoisation prevents chunks rerenders that could cause perfomance issues on big tables | ||
|
|
@@ -61,13 +62,18 @@ export const TableChunk = typedMemo(function TableChunk<T, F>({ | |
| shouldFetch, | ||
| shouldRender, | ||
| keepCache, | ||
| useColumnsIdsInRequest, | ||
| }: TableChunkProps<T, F>) { | ||
| const [isTimeoutActive, setIsTimeoutActive] = React.useState(true); | ||
| const [autoRefreshInterval] = useAutoRefreshInterval(); | ||
| const {noBatching} = usePaginatedTableState(); | ||
|
|
||
| //sort ids to prevent refetch if only order was changed | ||
| const columnsIds = columns.map((column) => column.name).toSorted(); | ||
| const columnsIds = React.useMemo( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may be if |
||
| () => | ||
| //sort ids to prevent refetch if only order was changed | ||
| useColumnsIdsInRequest ? columns.map((column) => column.name).toSorted() : undefined, | ||
| [columns, useColumnsIdsInRequest], | ||
| ); | ||
|
|
||
| const queryParams = { | ||
| offset: id * chunkSize, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ export interface TableChunksRendererProps<T, F> { | |
| renderEmptyDataMessage?: RenderEmptyDataMessage; | ||
| onDataFetched: (data?: PaginatedTableData<T>) => void; | ||
| keepCache: boolean; | ||
| useColumnsIdsInRequest?: boolean; | ||
| } | ||
|
|
||
| export const TableChunksRenderer = <T, F>({ | ||
|
|
@@ -47,6 +48,7 @@ export const TableChunksRenderer = <T, F>({ | |
| renderEmptyDataMessage, | ||
| onDataFetched, | ||
| keepCache, | ||
| useColumnsIdsInRequest, | ||
| }: TableChunksRendererProps<T, F>) => { | ||
| const chunkStates = useScrollBasedChunks({ | ||
| scrollContainerRef, | ||
|
|
@@ -125,6 +127,7 @@ export const TableChunksRenderer = <T, F>({ | |
| shouldFetch={chunkState.shouldFetch} | ||
| shouldRender={chunkState.shouldRender} | ||
| keepCache={keepCache} | ||
| useColumnsIdsInRequest={useColumnsIdsInRequest} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently we pass useColumnsIdsInRequest down the components tree to determine fetch options The question is - do we use the same entities table with and without useColumnsIdsInRequest simultenously? If not - I suggest to move this to fetching logics
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understood you correctly, you suggest not to pass the useColumnsIdsInRequest prop down the component tree, but instead decide whether to include columnsIds in the fetch options inside the fetching logic itself. Is that what you meant? |
||
| /> | ||
| ); | ||
| }, | ||
|
|
@@ -143,6 +146,7 @@ export const TableChunksRenderer = <T, F>({ | |
| rowHeight, | ||
| sortParams, | ||
| tableName, | ||
| useColumnsIdsInRequest, | ||
| ], | ||
| ); | ||
|
|
||
|
|
||
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 suggest to name this prop something like "preserveColsOrderInRequest"
useColumnsIdsInRequest is
looks like hook name
doesnt explicitly state how they are actually used