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

Support new labelRenderer and valueRenderer properties #18

Merged
merged 5 commits into from
Dec 31, 2015

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Dec 30, 2015

In order to support bvaughn/redux-devtools-filterable-log-monitor/issues/8, I need finer-grained control over what is rendered for node keys and values. So I set out to add 2 new properties: labelRenderer and valueRenderer.

Along the way I also noticed a lot of repetition between classes (JSONBooleanNode, JSONDateNode, JSONFunctionNode, JSONNullNode, JSONNumberNode, and JSONUndefinedNode- and even JSONArrayNode, JSONIterableNode, and JSONObjectNode). So I did some refactoring to tidy things up a bit. I hope I didn't take too much liberty by doing this.

I'd love to see this feature merged and I'd be willing to make any changes you feel necessary. Please feel free to reach out to me and we can chat. :)

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 30, 2015

Temporarily closing this PR while I investigate a performance issue. Will re-open once I've identified it.

@bvaughn bvaughn closed this Dec 30, 2015
@chibicode
Copy link
Contributor

@bvaughn thanks a lot! Keep me updated.

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 30, 2015

Happy to help. Will do! I noticed a performance decrease in the branch of bvaughn/redux-devtools-filterable-log-monitor that integrated with this update and I just wanted to rule out the change here before submitting. I'll keep you posted. :)

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 30, 2015

I identified the performance issue. My branch accidentally introduced a regression- all nodes were expanded by default instead of just the root. Easy enough to fix. :)

```js
<JSONTree
labelRenderer={raw => <strong>raw</strong>}
valueRenderer={raw => <em>raw</em>}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be <strong>{ raw }</strong> (same for em) instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops! Nice catch. I need to pay more attention to my code examples. :)

@chibicode
Copy link
Contributor

Added one comment but this looks good. Thank you so much for cleaning it up! (and say hi to Treasure Data folks)

I tried to run npm start under examples directory and it failed. The error was Can't add property context, object is not extensible. I'm guessing it's because this PR is dependent on one of React 0.14's features, and react-json-tree's examples is using React 0.13. Do you think that's the case?

I'm fine w/ dropping the support for 0.13. Just let me know if I should.

I'd also add this to examples/src/App.js:

<h3>More Fine Grained Rendering</h3>
<p>Pass <code>labelRenderer</code> or <code>valueRenderer</code>.</p>
<div>
  <JSONTree data={ data }
            labelRenderer={raw => <strong>{ raw }</strong>}
            valueRenderer={raw => <em>{ raw }</em>} />
</div>

…to fix an error. Added custom label/value renderer snippet to example App.
@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 31, 2015

Not a problem! Thanks for the useful library. :)

Hm. I'm unsure about the issue you pointed out. I don't specifically remember using any React 0.14.x features. That being said, updating the example package to React 0.14.x did resolve the problem- so if you don't mind me bumping that dependency with this PR- that would certainly be the most straight forward solution.

I updated the example App with the blurb you suggested. Thanks for that.

@chibicode
Copy link
Contributor

@bvaughn sounds good, thank you!

chibicode added a commit that referenced this pull request Dec 31, 2015
Support new `labelRenderer` and `valueRenderer` properties
@chibicode chibicode merged commit 5340074 into alexkuz:master Dec 31, 2015
@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 31, 2015

You're quite welcome! :)

@chibicode
Copy link
Contributor

@bvaughn published 0.4.0.

@bvaughn
Copy link
Contributor Author

bvaughn commented Dec 31, 2015

Thanks so much for the speedy update!

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