-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Try disabling async mode provider around selected block in ListView #34519
Conversation
Size Change: +19 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
const expand = useCallback( | ||
( clientId ) => { | ||
if ( ! clientId ) { | ||
return; | ||
} | ||
setExpandedState( { type: 'expand', clientId } ); | ||
}, | ||
[ setExpandedState ] | ||
); | ||
const collapse = useCallback( | ||
( clientId ) => { | ||
if ( ! clientId ) { | ||
return; | ||
} | ||
setExpandedState( { type: 'collapse', clientId } ); | ||
}, | ||
[ setExpandedState ] | ||
); |
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.
These are used in a context provider, so I think they should be memoized to stop the consumers from constantly updating.
@@ -125,7 +131,7 @@ function ListView( | |||
); | |||
|
|||
return ( | |||
<> | |||
<AsyncModeProvider value={ true }> |
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 suppose this could be <AsyncModeProvider value>
, but that seems a bit unusual.
@@ -94,7 +95,8 @@ export default function ListViewBranch( props ) { | |||
}; | |||
|
|||
return ( | |||
<Fragment key={ clientId }> | |||
// Make updates to the selected block synchronous. | |||
<AsyncModeProvider key={ clientId } value={ ! isSelected }> |
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.
Could be further enhanced using an IntersectionObserver
.
I noticed that in Safari, selecting blocks in the list view doesn't actually update the selected block there until you do another change in the UI (move focus or something else). I know that That said, it doesn't seem specific to this PR. |
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.
Clever use of sync rendering of selected branches ✨ I couldn't think of ways of breaking this (copy, paste, cut works for me), but I did want to note that depending on the drag implementation it is possible to modify the list without having items selected.
Maybe see if we can get a +1 review here, or potentially enable a typing performance test with the list view open.
Here's an example of what I see on this branch:
CleanShot.2021-09-08.at.12.41.56.mp4
Good spot on the Safari behavior @youknowriad. I added #34669 to note the issues in trunk. Are you still seeing update issues on this branch for Safari, this branch appears to clear it up for me: CleanShot.2021-09-08.at.14.20.25.mp4 |
f39549a
to
d17c44f
Compare
@gwwar I think I've solved the problem with dragging. The issue seemed to be that each block was doing its own check to determine whether it or a parent was being dragged (and using an expensive I also did a little bit of other refactoring to reduce prop drilling and move selectedClientIds and draggedClientIds to the context provider. |
} | ||
|
||
return { | ||
selectedClientIds: getSelectedBlockClientId(), |
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.
For other reviewers getSelectedBlockClientIds vs getSelectedBlockClientId
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.
Nice work @talldan ! I tested this and verified that I'm no longer seeing artifacts on delete/paste/drag. I think the simplification of not rendering a dragged branch makes a lot of sense. Here's what I see:
Description
Tries the suggestion in this comment to solve the slow updates in List View - #34477 (comment).
The problem seems to be that the main selector used to build the ListViewTree is wrapped in
<AsyncModeProvider value={ true }>
. This PR moves the AsyncModeProvider into an inner part of the ListView component so that this is no longer the case. Each block and its nested block is also wrapped in a<AsyncModeProvider>
with the select block being synchronous and unselected blocks being asynchronous.How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.js
files for terms that need renaming or removal).