Skip to content
This repository has been archived by the owner on May 14, 2020. It is now read-only.

Adds notification handler for node expansion #57

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

philliphoff
Copy link

Adds the optional property onNodeExpansionChanged(keyName, data, level, expanded) to JSONNestedNode. This allows applications to be notified when a node is expanded or collapsed. This can be used, for example, in conjunction with shouldExpandNode to preserve the expansion state of the JSON tree across views.

@@ -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.
Copy link
Owner

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?

Copy link
Author

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.

Copy link
Owner

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 to onExpansionArrowClicked (or something like that)
  • move onNodeExpansionChanged to componentDidUpdate (with checking this.state.expanded !== prevState.expanded), so it would be called every time expanded 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
Copy link
Owner

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.

Copy link
Author

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.

Copy link
Owner

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.

}
});
};

Copy link
Owner

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants