Adds notification handler for node expansion#57
Adds notification handler for node expansion#57philliphoff wants to merge 4 commits intoalexkuz:masterfrom
Conversation
| - `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.
Looks like overkill to me, TBH. Can we just leave onNodeExpansionChanged?
There was a problem hiding this comment.
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.
Then my suggestion is this:
- rename
onNodeExpansionChangingtoonExpansionArrowClicked(or something like that) - move
onNodeExpansionChangedtocomponentDidUpdate(with checkingthis.state.expanded !== prevState.expanded), so it would be called every timeexpandedwas changed, not just when user clicked it - I feel like it would be more consistent
| - `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.
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.
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.
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 withshouldExpandNodeto preserve the expansion state of the JSON tree across views.