Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] New API method:
setItemExpansion
#12595[TreeView] New API method:
setItemExpansion
#12595Changes from 1 commit
324c15c
a87c964
066bd84
6546d40
bef4f91
9f8678c
5330824
b918f4a
564b722
11c9481
faa8bca
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 took the opportunity to do this performance improvement now because in order to avoid code duplication between
toggleItemExpansion
andsetItemExpansion
, I need to check the expansion status twice and doing it with a linear complexity with awefull.@romgrk do you have a preference on the data structure to use for this model? Knowing that for now it's purely internal and we keep the array for the
expandedItems
prop (we can reconsider in the future).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.
@noraleonte I propose you to switch the logic for the definition of the instance / publicAPI and to instead define the method on
XXXPublicAPI
and useextend
onXXXInstance
.That way it's easy to be sure that every public method has a good JSDoc.
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's probably a much better way to do it 👍
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 DX question here is about the
event
Do we require the user to pass an event? This is needed right now to call the
onItemExpansionToggle
prop.If we don't, do we only call
onItemExpansionToggle
when the event is defined or do we make the event nullable ononItemExpansionToggle
?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 personally don't see a problem with keeping the event required here 🤔 I'm trying to think of a scenario where users would want to specifically avoid passing the event here 🤔
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.
We could imagine scenarios where there is no event (expand an item when receiving some data from the server for instance).
But I'd be in favor of keeping the event mandatory and making it nullable if we have feedback of valid use-cases that ask for it.