-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[TreeView] Do not re-render every Tree Item when the Rich Tree View re-renders (introduce selectors) #14210
[TreeView] Do not re-render every Tree Item when the Rich Tree View re-renders (introduce selectors) #14210
Conversation
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
RichTreeViewItemsContext.displayName = 'RichTreeViewItemsProvider'; | ||
} | ||
|
||
const WrappedTreeItem = React.memo(function WrappedTreeItem({ |
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 main logic to be able to memoize the item is here.
Now, it's the WrappedTreeItem
who accesses its meta data using a selector AND accesses its children using a selector.
That way, the props it passes to TreeItem
(which has React.memo
) only updates when there is an actual change.
We then use the RichTreeViewItemsContext
context to allow nested items to render there own children.
Note that the whole memoization falls appart when an inline slotProps.item
is provided 😬
I'll probably add a not on the doc, but I think it's not something we can avoid...
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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.
First batch of review: The new dx looks good to me. It seems solid and I like that it fixes some internal hacks along the way 👌
|
||
const childrenNumber = publicAPI.getItemOrderedChildrenIds(props.itemId).length; | ||
const selectFirstChildren = status.expanded |
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 editedItemRef = React.useRef(state.editedItemId); | ||
|
||
const isItemBeingEditedRef = (itemId: TreeViewItemId) => editedItemRef.current === itemId; |
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.
That simplifies things a lot. I had a similar issue on the lazy loading when the fetching failed, I needed to undo the expansion of the item. My solution is a bit hacky, but this should help 👍
return typeof params.isItemEditable === 'function' | ||
? params.isItemEditable(item) | ||
: Boolean(params.isItemEditable); | ||
store.update((prevState) => ({ ...prevState, label: { editedItemId } })); |
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.
Love how it simplified this part too
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 went through the docs and I don't see any weird/buggy behavior 💪 Left some super small nitpicks, but other than that, everything looks good to me 👌 Congrats on this huge effort, it's super valuable 🚀
docs/data/tree-view/tree-item-customization/tree-item-customization.md
Outdated
Show resolved
Hide resolved
@@ -34,9 +41,21 @@ const CustomTreeItem = React.forwardRef(function CustomTreeItem( | |||
props: TreeItemProps, | |||
ref: React.Ref<HTMLLIElement>, | |||
) { | |||
const { publicAPI } = useTreeItem(props); | |||
const { publicAPI, status } = useTreeItem(props); |
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.
unrelated to this PR, but I just realized we can do the same thing with useTreeItemUtils
too 🤔 Could become confusing for user
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.
True, I think it will be solved when we migrate the Tree Item to the Base UI DX since useTreeItem
will go away.
docs/data/tree-view/tree-item-customization/useTreeItemModelHook.tsx
Outdated
Show resolved
Hide resolved
|
||
```ts | ||
function CustomTreeItem(props) { | ||
const { publicAPI } = useTreeItemUtils(); |
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 looks like a typo, shouldn't it be
const { publicAPI } = useTreeItemUtils(); | |
const { publicApi } = useTreeItemUtils(); |
per the rest of the API conversion? We could add this to https://mui.com/material-ui/guides/api/.
Off-topic: Seeing "public" in the name feels a bit strange, it feels a bit like a tautology: if it's documented, it's public.
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.
Both remark are totally valid.
It's publicAPI
( / publicApi
) internally, but when exposed it should probably be api
Part of #14200
Work
Internal changes
itemMetaMap
=>itemMetaLookup
to make clear it is an object and not aMap
(now that we do have map in the state)itemMap
=>itemModelLookup
to make clear it is an object and not aMap
(now that we do have map in the state) and to be more precise about what it containsitemOrderedChildrenIds
=>itemOrderedChildrenIdsLookup
to make its purpose more clearitemChildrenIndexes
=>itemChildrenIndexesLookup
to make its purpose more cleardisabledItemsFocusabflaviendelanglele
to the state instead of passing it to thecontextValue
, it allows droppinginstance.isItemNavigable
in favor of a selectorfocus
key for consistencylabel
key for consistencyselectedItemsMap
in the state to be usable in selectorsexpandedItemsMap
in the state to be usable in selectorsdefaultFocusableItemId
in the state to be usable in selectorsIntroduce selectors
Replace the state with a store stored in a ref
Create selectors for every state access
The selectors are defined by each plugin in a
useTreeViewXXX.selectors.ts
file:Replace every
instance
call in the render with selectorsBREAKING: Add
idAttribute
toTreeItemProviderProps
(to stop getting it fromitemMeta
, see [TreeView] Do not re-render every Tree Item when the Rich Tree View re-renders (introduce selectors) #14210 (comment) for context)Add memoization
React.memo
around theWrappedTreeItem
component inRichTreeViewItems
Docs
publicAPI
in the renderExtracted PRs
getDefaultizedParams
instead of in the plugin render #14661instance.getTreeItemIdAttribute
#14667Changelog
Breaking changes
publicAPI
methods in therender
because they are now memoized — Learn more.