feat(DualListSelector): add composable tree example#6652
feat(DualListSelector): add composable tree example#6652tlabaj merged 6 commits intopatternfly:mainfrom
Conversation
| filterValue && descendentsOnThisPane.some(id => memoizedNodeText[id].includes(filterValue)); | ||
| const isFilterMatch = filterValue && node.text.includes(filterValue) && descendentsOnThisPane.length > 0; | ||
|
|
||
| // A node is displayed if: |
There was a problem hiding this comment.
nit. can you update comment to say if either is true just to be more clear
| const { memoizedLeavesById, memoizedAllLeaves, memoizedNodeText } = React.useMemo(() => { | ||
| let leavesById = {}; | ||
| let allLeaves = []; | ||
| let nodeTexts = {}; | ||
| data.forEach(foodNode => { | ||
| nodeTexts = { ...nodeTexts, ...buildTextById(foodNode) }; | ||
| leavesById = { ...leavesById, ...getLeavesById(foodNode) }; | ||
| allLeaves = [...allLeaves, ...getDescendantLeafIds(foodNode)]; | ||
| }); | ||
| return { | ||
| memoizedLeavesById: leavesById, | ||
| memoizedAllLeaves: allLeaves, | ||
| memoizedNodeText: nodeTexts | ||
| }; | ||
| }, [data]); |
There was a problem hiding this comment.
Shouldn't this be in the form of:
const memoizeNodes = (data: FoodNode[]) => {
let leavesById = {};
let allLeaves = [];
let nodeTexts = {};
data.forEach(foodNode => {
nodeTexts = { ...nodeTexts, ...buildTextById(foodNode) };
leavesById = { ...leavesById, ...getLeavesById(foodNode) };
allLeaves = [...allLeaves, ...getDescendantLeafIds(foodNode)];
});
return {
memoizedLeavesById: leavesById,
memoizedAllLeaves: allLeaves,
memoizedNodeText: nodeTexts
};
};
// Builds a map of child leaf nodes by node id - memoized so that it only rebuilds the list if the data changes.
const { memoizedLeavesById, memoizedAllLeaves, memoizedNodeText } = React.useMemo(() => memoizeNodes(data), [data]);with data being passed as an argument to the memoization callback?
This is a genuine question as I'm not well versed with React memoization and I'm not sure how to really test this without a large dataset, I just saw that in the React docs they show usage as:
There was a problem hiding this comment.
My understanding is that React.useMemo works like React.useEffect where it watches for changes in the dependencies listed in the dependency array at the end - and it only recalculates the value of the memoizedValue if those dependencies change.
There was a problem hiding this comment.
I misunderstood something from the docs and confused myself, disregard 😅
There was a problem hiding this comment.
@wise-king-sullyman I think both are correct. Defining a dedicated memoizeNodes function at render time just before passing useMemo an anonymous function that calls it behaves the same as defining that behavior directly in the anonymous function. The important part is the [data] dependency array which will prevent the whole anonymous function from being run again if the data didn't change. So it's a matter of preference - I think the example in the React docs was written that way for brevity.
(edit: sorry, i didn't see your last comment before writing this!)
| const [chosenLeafIds, setChosenLeafIds] = React.useState<string[]>(['beans', 'beef', 'chicken', 'tofu']); | ||
| const [chosenFilter, setChosenFilter] = React.useState<string>(''); | ||
| const [availableFilter, setAvailableFilter] = React.useState<string>(''); | ||
| let hiddenChosen = [] as string[]; |
There was a problem hiding this comment.
Nit, but I think you can do let hiddenChosen: string[] = []; to avoid the use of the as keyword (I often search for that and try to remove it since it can be a code smell, even though it isn't here)
| const { memoizedLeavesById, memoizedAllLeaves, memoizedNodeText } = React.useMemo(() => { | ||
| let leavesById = {}; | ||
| let allLeaves = []; | ||
| let nodeTexts = {}; | ||
| data.forEach(foodNode => { | ||
| nodeTexts = { ...nodeTexts, ...buildTextById(foodNode) }; | ||
| leavesById = { ...leavesById, ...getLeavesById(foodNode) }; | ||
| allLeaves = [...allLeaves, ...getDescendantLeafIds(foodNode)]; | ||
| }); | ||
| return { | ||
| memoizedLeavesById: leavesById, | ||
| memoizedAllLeaves: allLeaves, | ||
| memoizedNodeText: nodeTexts | ||
| }; | ||
| }, [data]); |
There was a problem hiding this comment.
@wise-king-sullyman I think both are correct. Defining a dedicated memoizeNodes function at render time just before passing useMemo an anonymous function that calls it behaves the same as defining that behavior directly in the anonymous function. The important part is the [data] dependency array which will prevent the whole anonymous function from being run again if the data didn't change. So it's a matter of preference - I think the example in the React docs was written that way for brevity.
(edit: sorry, i didn't see your last comment before writing this!)
wise-king-sullyman
left a comment
There was a problem hiding this comment.
I have one small suggestion but this LGTM overall so I'm comfortable approving whether you change it or not.
| ...(isDisplayed | ||
| ? [ | ||
| { | ||
| id: node.id, | ||
| text: node.text, | ||
| isChecked, | ||
| checkProps: { 'aria-label': `Select ${node.text}` }, | ||
| hasBadge: node.children && node.children.length > 0, | ||
| badgeProps: { isRead: true }, | ||
| defaultExpanded: isChosen ? !!chosenFilter : !!availableFilter, | ||
| children: node.children | ||
| ? buildOptions(isChosen, node.children, isFilterMatch || hasParentMatch) | ||
| : undefined | ||
| } | ||
| ] | ||
| : []), | ||
| ...(!isDisplayed && node.children && node.children.length | ||
| ? buildOptions(isChosen, node.children, hasParentMatch) | ||
| : []), | ||
| ...(remainingNodes ? buildOptions(isChosen, remainingNodes, hasParentMatch) : []) | ||
| ]; |
There was a problem hiding this comment.
This is pretty dense, I think some newlines between the spreads would improve readability if nothing else.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #6401, Closes #6424