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

Add element's key and ref to tree and prop views #328

Merged

Conversation

Timothee
Copy link
Contributor

Fixes #148.
Fixes #226.

If a key or ref is set on the React element, it's displayed before
the props in the tree view and in the PropState sidebar.

If none is set, nothing is changed.

Also removed a piece of code Node.js that would never run. (condition
is checked in the previous block that returns)

Fixes facebook#148.
Fixes facebook#226.

If a `key` or `ref` is set on the React element, it's displayed before
the `props` in the tree view and in the `PropState` sidebar.

If none is set, nothing is changed.

Also removed a piece of code `Node.js` that would never run. (condition
is checked in the previous block that `return`s)
@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@Timothee
Copy link
Contributor Author

CLA signed.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@Timothee
Copy link
Contributor Author

@jaredly @kassens anything I can do to get this PR looked at? Thanks!

@Timothee
Copy link
Contributor Author

Timothee commented Apr 8, 2016

@spicyj any chance to get this looked at? Thanks!

@Timothee
Copy link
Contributor Author

😐

@jaredly
Copy link
Contributor

jaredly commented Apr 29, 2016

@Timothee sorry to take so long on this. It looks great! one request: can you make it handle the "ref is a function" case gracefully?
Also, make sure to coerce key to a string -- it's supposed to be, but in practice it might not be

@Timothee
Copy link
Contributor Author

React also supports using a string (instead of a callback) as a ref prop on any component, although this approach is mostly legacy at this point

I suppose I've been using legacy code with regards to refs then 😄

@jaredly if ref is a function, should I just not display anything at all? From the examples, it seems that with a function, there's nothing to display really…

@Timothee
Copy link
Contributor Author

Actually, re-reading #226:

functions don't have a meaningful representation in the sidebar

but

we might be able to get a meaningful representation of functions from name and displayName, or if the VM permits, even line number + source link

I'll look into that last one a bit, but otherwise will just skip it if it's a function. (unless you have better ideas)

@jaredly
Copy link
Contributor

jaredly commented Apr 29, 2016

that sounds fine to me!

Refs as strings is the "old" way. They can also be functions and this
should handle it nicely. They are already handled nicely in the main
elements panel. However the sidebar only showed `[object Object]`.

To make it work, I reused the `PropVal` element since it already manages
styles for different types of values. This way, `key` and `ref` look
like they do in the main panel.

In particular, `PropVal` handles functions well: if they are named, they
are replaced by `functionName()`, otherwise simply by `fn()`.

This change also includes parsing `key` as a string, just in case.
@Timothee
Copy link
Contributor Author

@jaredly updated. Now the following JSX:

var Container = React.createClass({
    render: () => {
        return (
            <div>
                <div key='withKey'>with a key</div>
                <div ref='stringRef'>with a string ref</div>
                <div ref={()=>{}}>with an unnamed function ref</div>
                <div ref={function namedFn() {}}>with a named function ref</div>
                <div key='withKeyAndRef' ref={function namedFn() {}}>with a named function ref and a key</div>
            </div>
        );
    }
});

is shown like this:

screen shot 2016-04-29 at 1 16 39 pm

The replacement of the unnamed function into fn() is handled by PropVal.

Timothée Boucher added 3 commits April 29, 2016 22:26
These `Props` can probably use a `key`, sitting like that in a row...
Moving the triangle up a bit to line it up better with the text.
@montogeek
Copy link

Please merge this :)

@Timothee
Copy link
Contributor Author

@jaredly bump 👀

@gaearon gaearon merged commit d83931e into facebook:master May 19, 2016
@gaearon
Copy link
Contributor

gaearon commented May 19, 2016

This is in. Thank you so much!

@montogeek
Copy link

montogeek commented May 19, 2016

THANKS!
@gaearon It's automatically available in the Chrome Store?

@Timothee Timothee deleted the add-key-and-ref-to-tree-and-panel-view branch May 19, 2016 16:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants