-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[DataGrid] Refactor rendering, remove rafUpdate #532
Merged
Merged
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
abe63f3
refactored rendering, removed rafUpdate
dtassone 7dd3cd5
fix lint
dtassone 6520205
Update packages/grid/_modules_/grid/models/api/stateApi.ts
dtassone 9ed2d05
cleanup
dtassone dc1fe69
cleanup
dtassone 8e2e989
refactor window to ownerdoc...
dtassone 3eb1537
Update packages/grid/_modules_/grid/components/AutoSizer.tsx
dtassone 7af936c
Update packages/grid/_modules_/grid/hooks/features/virtualization/use…
dtassone 99a1833
small refact
dtassone fcadbe0
Merge branch 'fixTests' of github.com:dtassone/material-ui-x into fix…
dtassone 254d070
fix bad commit review
dtassone d85aa37
better story
dtassone 6d38055
revert refactoring as breaking the tests
dtassone e0289cd
prettier
dtassone fa9bc7f
slow down resize debounce
dtassone cf0aa5e
make sure the tests don't rely on the resize event
oliviertassinari bd72f93
no any
oliviertassinari 58ee78c
fine grain timer in test
oliviertassinari 112b830
hedge against weak timing guarentees of setTimeout
oliviertassinari File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 changes: 3 additions & 0 deletions
3
packages/grid/_modules_/grid/hooks/features/core/useGridApi.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 changes: 5 additions & 6 deletions
11
packages/grid/_modules_/grid/hooks/features/core/useGridState.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,21 @@ | ||
import * as React from 'react'; | ||
import { ApiRef } from '../../../models/api/apiRef'; | ||
import { useRafUpdate } from '../../utils/useRafUpdate'; | ||
import { GridState } from './gridState'; | ||
import { useGridApi } from './useGridApi'; | ||
|
||
export const useGridState = ( | ||
apiRef: ApiRef, | ||
): [GridState, (stateUpdaterFn: (oldState: GridState) => GridState) => void, () => void] => { | ||
const api = useGridApi(apiRef); | ||
const [, forceUpdate] = React.useState(); | ||
const [rafUpdate] = useRafUpdate(apiRef, () => forceUpdate((p: any) => !p)); | ||
|
||
const forceUpdate = React.useCallback( | ||
() => apiRef.current.forceUpdate(() => apiRef.current.state), | ||
dtassone marked this conversation as resolved.
Show resolved
Hide resolved
|
||
[apiRef], | ||
); | ||
const setGridState = React.useCallback( | ||
(stateUpdaterFn: (oldState: GridState) => GridState) => { | ||
api.state = stateUpdaterFn(api.state); | ||
}, | ||
[api], | ||
); | ||
|
||
return [api.state, setGridState, rafUpdate]; | ||
return [api.state, setGridState, forceUpdate]; | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,2 @@ | ||
export * from './useLogger'; | ||
export * from './useRafUpdate'; | ||
export * from './useScrollFn'; |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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? I prefer to useState so I can pass the state I need...
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 current usage force you to call forceUpdate with an argument, and it has to have a unique reference, like a new object or a state setting that returns a unique reference. In the current logic, it's called with a function. It could be simpler if it didn't need to accept an argument, and maybe less error prone?
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.
it does, you just need to pass a function
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 name
forceUpdate
is misleading. You have to pass the correct parameter to schedule work.Though generally forcing a re-render is rarely needed. Whenever you
forceUpdate
why can't you move whatever changed into state?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.
There is one case that took me some time to understand, I think that by doing
We can skip updates when the state hasn't changed, which can be an interesting feature. I was confused by the name, we definitely need to find a different one. IMHO
forceUpdate
=== no arguments. What aboutsetState
?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.
this is not a setState, it's just to force the ui to rerender
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.
@dtassone Are you saying that all we need is a new reference? If its the case, I would rather do:
to remove the dependency another part of the logic.
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.
But it is? Assuming you refer to
setState
returned from theuseState
hook then I don't see any difference. BothsetState
andforceUpdate
have to know what the current state is to actually force a re-rerender (which isn't clear why you need this).