-
Notifications
You must be signed in to change notification settings - Fork 115
Adds notification handler for node expansion #57
base: master
Are you sure you want to change the base?
Adds notification handler for node expansion #57
Conversation
@@ -134,6 +134,9 @@ For `labelRenderer`, you can provide a full path - [see this PR](https://github. | |||
- `shouldExpandNode: function(keyName, data, level)` - determines if node should be expanded (root is expanded by default) | |||
- `hideRoot: Boolean` - if `true`, the root node is hidden. | |||
- `sortObjectKeys: Boolean | function(a, b)` - sorts object keys with compare function (optional). Isn't applied to iterable maps like `Immutable.Map`. | |||
- `onNodeExpansionChanging: function(keyName, data, level, expanded)` - invoked when a node is expanding or collapsing. | |||
- `onNodeExpansionChanged: function(keyName, data, level, expanded)` - invoked when a node is expanded or collapsed. |
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 like overkill to me, TBH. Can we just leave onNodeExpansionChanged
?
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.
It turned out that onNodeExpansionChanged
didn't actually work for our purposes, due to React's deferral/batching of state changes; we saw occasional race conditions in the process of re-rendering before the state change had actually occurred. I added onNodeExpansionChanging
to provide more immediate notification, but kept both callbacks for the sake of symmetry, that onNodeExpansionChanged
might still be useful for others that don't have as specific a need as us in terms of timing of the notification, and it being probably the "most correct" from a React perspective.
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.
Then my suggestion is this:
- rename
onNodeExpansionChanging
toonExpansionArrowClicked
(or something like that) - move
onNodeExpansionChanged
tocomponentDidUpdate
(with checkingthis.state.expanded !== prevState.expanded
), so it would be called every timeexpanded
was changed, not just when user clicked it - I feel like it would be more consistent
@@ -134,6 +134,9 @@ For `labelRenderer`, you can provide a full path - [see this PR](https://github. | |||
- `shouldExpandNode: function(keyName, data, level)` - determines if node should be expanded (root is expanded by default) | |||
- `hideRoot: Boolean` - if `true`, the root node is hidden. | |||
- `sortObjectKeys: Boolean | function(a, b)` - sorts object keys with compare function (optional). Isn't applied to iterable maps like `Immutable.Map`. | |||
- `onNodeExpansionChanging: function(keyName, data, level, expanded)` - invoked when a node is expanding or collapsing. | |||
- `onNodeExpansionChanged: function(keyName, data, level, expanded)` - invoked when a node is expanded or collapsed. | |||
- `isNodeExpansionDynamic: Boolean` - if `true`, `shouldExpandNode` will be called each render to determine the expansion state of a node, rather than just once during mount |
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 one won't be necessary with #61 I suppose.
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.
My understanding of #61 is that it allows the re-evaluation of shouldExpandNode()
whenever the node's associated data
(or other) property changes, not necessarily upon a re-render (or the result of non-node-related application state changes). This works when the tree's bound object is completely swapped with another (e.g. a trimmed version of the original) which is the example described in #61. I'm not sure it works when the bound object doesn't itself change, but the expansion state of those existing nodes do change.
isNodeExpansionDynamic
was added to address any back-compatibility concern with a new version of the tree suddenly calling shouldExpandNode()
more often, which could be a performance or correctness issue if that function is not performant or has side-effects. (Obviously the function should performant and ideally not have side-effects, but we don't have control over what people actually do and should--when reasonably possible--try to avoid compounding any such issues.) #61 addresses that to some extent by only re-invoking shouldExpandNode()
only if certain properties (like data
) change.
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.
If you need it to be called on each render, you can create a new callback every time, for example:
shouldExpandNode={this.handleShouldExpandNode.bind(this)}
instead of:
shouldExpandNode={this.handleShouldExpandNode}
(considering handleShouldExpandNode
is declared as a class prop)
It might be not that obvious (and should be mentioned in docs), but I think it's fair - shouldExpandNode
is considered a pure function, and if it's not - just wrap it in a new one.
} | ||
}); | ||
}; | ||
|
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.
After #61 it's better to move this back to class property.
Adds the optional property
onNodeExpansionChanged(keyName, data, level, expanded)
toJSONNestedNode
. This allows applications to be notified when a node is expanded or collapsed. This can be used, for example, in conjunction withshouldExpandNode
to preserve the expansion state of the JSON tree across views.