-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix(Tree): use context to supply generated props to items #12182
fix(Tree): use context to supply generated props to items #12182
Conversation
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: d2abb5be48d5bdd44bcd2d593d31441fe45d35e8 (build) |
|
||
return [...renderedItems, renderedItem, ...(isSubtreeExpanded ? renderItems(item['items']) : ([] as any))]; | ||
const elementRef = React.createRef<HTMLElement>(); | ||
const contextValue: TreeItemRenderContextValue = { |
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 will not work 😨 https://smykhailov.github.io/react-patterns/#/context
So, even if consumers will inject React.memo()
around TreeItem
they will still receive updates as context value will always change.
const contextValue: TreeItemRenderContextValue = { | ||
level, | ||
index: index + 1, | ||
size: items.length, |
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 don't see any benefits from passing level
, index
& size
via context as they are plain.
I proposed to use Context because I want to avoid creation of new functions in overrideProps
, it's done ✔️
Now we need:
- find a way to avoid
elementRef
,onSiblingsExpand
,onFocusFirstChild
,onFocusParent
creation on each render, i.e. remove dependency on exact item TreeItemContext
shouldTreeContext
, we obviously don't want to wrap each item with ownProvider
, it will be easy doable after previous item will be solved
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
|
||
setActiveItemIds = (e: React.SyntheticEvent, activeItemIds: string[]) => { | ||
_.invoke(this.props, 'onActiveItemIds', e, { ...this.props, activeItemIds }); | ||
_.invoke(this.props, 'onActiveItemIdsChange', e, { ...this.props, activeItemIds }); |
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.
sometimes my coding is simply bad.
}), | ||
overrideProps: this.handleTreeItemOverrides | ||
parent, | ||
level, |
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 want to refactor level, index, treeSize in a subsequent PR. I think it makes sense to just call them ariaPosInSet, ariaSetSize, ariaLevel since that's their only use. Opinions?
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.
aria-level
in this case. But makes sense only if they are used only for attributes...
onTitleClick: (e: React.SyntheticEvent, treeItemProps: TreeItemProps) => void; | ||
} | ||
|
||
export const TreeItemContext = React.createContext<TreeItemRenderContextValue>(null); |
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.
export const TreeItemContext = React.createContext<TreeItemRenderContextValue>(null); | |
export const TreeContext = React.createContext<TreeContextValue>(null); |
I would say that this is TreeContext
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.
Cool!
onFocusFirstChild: (itemId: string) => void; | ||
onFocusParent: (itemId: string) => void; | ||
onSiblingsExpand: (e: React.SyntheticEvent, itemProps: TreeItemProps) => void; | ||
onTitleClick: (e: React.SyntheticEvent, treeItemProps: TreeItemProps) => void; |
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.
onTitleClick: (e: React.SyntheticEvent, treeItemProps: TreeItemProps) => void; | |
onTitleClick: (e: React.SyntheticEvent, itemProps: TreeItemProps) => void; |
nit
Pull request checklist
$ yarn change
Description of changes
renderItems
.Fixes microsoft/fluent-ui-react#2295.
Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow