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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

- `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.


### Credits

Expand Down
18 changes: 18 additions & 0 deletions examples/src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,24 @@ const App = () => (
shouldExpandNode={() => false}
/>
</div>
<p>Expansion Notifications</p>
<div>
<JSONTree
data={data}
theme={theme}
shouldExpandNode={() => false}
onNodeExpansionChanging={
(keyPath, data, level, expanded) => {
console.log(`Node ${expanded ? 'expanding' : 'collapsing'}: ${keyPath}`);
}
}
onNodeExpansionChanged={
(keyPath, data, level, expanded) => {
console.log(`Node ${expanded ? 'expanded' : 'collapsed'}: ${keyPath}`);
}
}
/>
</div>
</div>
);

Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@
"author": "Shu Uesugi <shu@chibicode.com> (http://github.com/chibicode)",
"contributors": [
"Dave Vedder <veddermatic@gmail.com> (http://www.eskimospy.com/)",
"Daniele Zannotti <dzannotti@me.com> (http://www.github.com/dzannotti)"
"Daniele Zannotti <dzannotti@me.com> (http://www.github.com/dzannotti)",
"Phillip Hoff <phillip@orst.edu> (http://www.github.com/philliphoff)"
],
"license": "MIT",
"bugs": {
Expand Down
47 changes: 39 additions & 8 deletions src/JSONNestedNode.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,10 @@ export default class JSONNestedNode extends React.Component {
level: PropTypes.number.isRequired,
sortObjectKeys: PropTypes.oneOfType([PropTypes.func, PropTypes.bool]),
isCircular: PropTypes.bool,
expandable: PropTypes.bool
expandable: PropTypes.bool,
onNodeExpansionChanging: PropTypes.func,
onNodeExpansionChanged: PropTypes.func,
isNodeExpansionDynamic: PropTypes.bool
};

static defaultProps = {
Expand All @@ -98,7 +101,7 @@ export default class JSONNestedNode extends React.Component {
};
}

shouldComponentUpdate = shouldPureComponentUpdate;
shouldComponentUpdate = this.props.isNodeExpansionDynamic ? undefined : shouldPureComponentUpdate;

render() {
const {
Expand All @@ -114,7 +117,13 @@ export default class JSONNestedNode extends React.Component {
labelRenderer,
expandable
} = this.props;
const expanded = this.state.expanded;
const expanded =
this.props.isNodeExpansionDynamic
&& this.props.shouldExpandNode
&& !this.props.isCircular
? this.props.shouldExpandNode(this.props.keyPath, this.props.data, this.props.level)
: this.state.expanded;

const renderedChildren = expanded || (hideRoot && this.props.level === 0) ?
renderChildNodes({ ...this.props, level: this.props.level + 1 }) : null;

Expand All @@ -131,6 +140,30 @@ export default class JSONNestedNode extends React.Component {
);
const stylingArgs = [keyPath, nodeType, expanded, expandable];

const handleClick = () => {
const newState = !expanded;

if (this.props.onNodeExpansionChanging) {
this.props.onNodeExpansionChanging(
this.props.keyPath,
this.props.data,
this.props.level,
newState);
}

this.setState(
{ expanded: newState },
() => {
if (this.props.onNodeExpansionChanged) {
this.props.onNodeExpansionChanged(
this.props.keyPath,
this.props.data,
this.props.level,
this.state.expanded);
}
});
};

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.

return hideRoot ? (
<li {...styling('rootNode', ...stylingArgs)}>
<ul {...styling('rootNodeChildren', ...stylingArgs)}>
Expand All @@ -144,18 +177,18 @@ export default class JSONNestedNode extends React.Component {
styling={styling}
nodeType={nodeType}
expanded={expanded}
onClick={this.handleClick}
onClick={handleClick}
/>
}
<label
{...styling(['label', 'nestedNodeLabel'], ...stylingArgs)}
onClick={expandable && this.handleClick}
onClick={expandable && handleClick}
>
{labelRenderer(...stylingArgs)}
</label>
<span
{...styling('nestedNodeItemString', ...stylingArgs)}
onClick={expandable && this.handleClick}
onClick={expandable && handleClick}
>
{renderedItemString}
</span>
Expand All @@ -165,6 +198,4 @@ export default class JSONNestedNode extends React.Component {
</li>
);
}

handleClick = () => this.setState({ expanded: !this.state.expanded });
}