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: Allow expanded state to be controlled #2373
Conversation
Co-authored-by: Josh Black <joshblack@github.com>
Co-authored-by: Josh Black <joshblack@users.noreply.github.com>
Co-authored-by: Josh Black <joshblack@github.com>
🦋 Changeset detectedLatest commit: 4750160 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
…nto treeview-control-expanded
description="The controlled expanded state of item. Must be used in conjunction with onExpandedChange." | ||
/> | ||
<PropsTableRow | ||
name="onExpandedChange" |
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.
Is there a good rule of thumb to follow for when to use on<State>Change(value)
, as in onExpandedChange()
, versus onChange(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.
Looks great! Left a couple comments around ideas for refactoring if they're helpful, totally get if not 👍
src/TreeView/TreeView.tsx
Outdated
}) | ||
|
||
React.useLayoutEffect(() => { | ||
setParentContainsCurrentRef.current = setParentContainsCurrent |
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.
With setContainsCurrentItem
coming from React.useState()
, we might be able to rely on that being stable instead of having to create a stable reference.
src/TreeView/TreeView.tsx
Outdated
const setParentContainsCurrentRef = React.useRef(setParentContainsCurrent) | ||
|
||
React.useLayoutEffect(() => { | ||
setIsExpandedRef.current = setIsExpanded |
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 bet we could update useControllableState
to provide a stable reference for the state setter to match useState()
if that would help out here with not having to mess around with stable refs
src/hooks/useControllableState.ts
Outdated
controlled.current = value !== undefined | ||
} | ||
|
||
function setState(stateOrUpdater: T | ((prevState: T) => T)) { |
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.
Idea from the stable state setter comment, this might be able to become:
const savedOnChange = React.useRef(onChange);
React.useEffect(() => {
savedOnChange.current = onChange;
});
const setState = React.useCallback((stateOrUpdater: T | ((prevState: T) => T)) => {
const value =
typeof stateOrUpdater === 'function'
? // @ts-ignore stateOrUpdater is a function
stateOrUpdater(state)
: stateOrUpdater
if (controlled.current === false) {
internalSetState(value)
}
if (onChange) {
savedOnChange.current?.(value)
}
}, [state]);
// --snip--
return [state, setState];
It'd be great to also get rid of state
from the dep array but I didn't see a quick way to do so 🤔
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 great feedback. Thank you! Is this what you had in mind? eeaf486
Summary
Allows the
expanded
state of tree items to be controlled (as requested by @jdrush89 from Repos). Example:I created a new story to demonstrate how you might use the controlled behavior:
👉 Try it out
CleanShot.2022-09-26.at.13.35.41.mp4
Shoutouts
Shoutout to @joshblack for writing the
useControllableState
hook 💖Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.