-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. My understanding of #61 is that it allows the re-evaluation of
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 It might be not that obvious (and should be mentioned in docs), but I think it's fair - |
||
|
||
### Credits | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = { | ||
|
@@ -98,7 +101,7 @@ export default class JSONNestedNode extends React.Component { | |
}; | ||
} | ||
|
||
shouldComponentUpdate = shouldPureComponentUpdate; | ||
shouldComponentUpdate = this.props.isNodeExpansionDynamic ? undefined : shouldPureComponentUpdate; | ||
|
||
render() { | ||
const { | ||
|
@@ -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; | ||
|
||
|
@@ -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); | ||
} | ||
}); | ||
}; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)}> | ||
|
@@ -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> | ||
|
@@ -165,6 +198,4 @@ export default class JSONNestedNode extends React.Component { | |
</li> | ||
); | ||
} | ||
|
||
handleClick = () => this.setState({ expanded: !this.state.expanded }); | ||
} |
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 addedonNodeExpansionChanging
to provide more immediate notification, but kept both callbacks for the sake of symmetry, thatonNodeExpansionChanged
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:
onNodeExpansionChanging
toonExpansionArrowClicked
(or something like that)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