[React Native] Add box inspect to NativeStyler by jhen0409 · Pull Request #590 · facebook/react-devtools

jhen0409 · 2017-03-20T09:22:37Z

In React Native, it have the built-in inspector can show the component width / height / margin / padding information:

The built-in inspector contents will hidden when we use react-devtools, this PR made the box inspector showing in React Native Style Editor:

jaredly · 2017-03-31T01:53:54Z

coool I've definitely wanted this

suchipi · 2017-04-10T05:48:19Z

+  top: number,
+  width: number,
+  height: number,
+  margin: Object,


Looks like these margin and padding objects always need to contain { top: number, left: number, right: number, bottom: number } (since they are given to Box). If you typed them as such within Props, it'd make it clearer what kind of props you need to pass to a BoxInspector.

Also, If you want, you could extract a type with a name like BoxMeasurements or something, and share it between both Props and BoxProps.

suchipi · 2017-04-10T05:52:24Z

+
+class BoxInspector extends React.Component {
+  props: Props;
+  defaultProps: DefaultProps;


This is telling flow that an instance of BoxInspector can have a property called defaultProps on it (the instance) of type DefaultProps. This seems incorrect; you normally only have a defaultProps property on the class, not the instance. In addition, I'm not sure what value typing defaultProps as DefaultProps adds, since DefaultProps is {}. Am I missing something here?

suchipi · 2017-04-10T05:54:58Z

-    this.props.bridge.call('rn-style:get', this.props.id, style => {
-      this.setState({style});
-    });
+    (this:any)._styleGet = this._styleGet.bind(this);


Why do you have to annotate this as any here?

Just avoid Property not found error of Flow, but I should define _styleGet: () => void; instead.

suchipi · 2017-04-10T05:57:58Z

    }
    this.props.bridge.send('rn-style:set', {id: this.props.id, attr, val});
-    this.setState({style: this.state.style});
+    this.setState(this.state.style);


This doesn't look right. You're copying properties that are on the style object into the state itself.

It appears that this was happening before to force a re-render because the style object was mutated. This will still force a re-render, but adding a bunch of unrelated properties to the state is likely not what you are trying to do.

suchipi · 2017-04-10T06:12:26Z

+};
+
+var Box = (props: BoxProps) => {
+  var box = props.box;


It's kinda weird to have a Box that takes a prop called box. It seems like there is a name for a domain concept here that wants to be found. Maybe creating a type for { top: number, left: number, right: number, bottom: number } will help us find a clearer name to use for this prop.

Alternatively, perhaps top, left, right, and bottom could be made top-level props on the element itself:

<Box top={4} left={5} right={6} bottom={7} />

suchipi · 2017-04-10T06:15:20Z

    style[newName] = val;
    this.props.bridge.send('rn-style:rename', {id: this.props.id, oldName, newName, val});
-    this.setState({style});
+    this.setState(this.state.style);


The above comment applies to this line as well (same situation).

gaearon · 2017-04-20T12:44:29Z

Do you think there is any harm in just inlining resolveBoxStyle implementation? It doesn’t seem to have anything RN-specific per se (correct me if I’m wrong!) I’d like to avoid expanding API surface if we can just get by with some duplication.

jhen0409 · 2017-04-20T14:49:06Z

Ahh, yeah it haven't anything RN-specific, mirror it will be more cleaner and support current RN version.

I pushed new commits.

gaearon · 2017-04-20T14:57:00Z

  var bridge = new Bridge(wall);
  var agent = new Agent(window, {
    rnStyle: !!resolveRNStyle,
+    rnStyleMeasure: !!resolveRNStyle,


We don't need this anymore, do we? Seems like it would always be supported if rnStyle is enabled.

I added measureSupport prop in NativeStyler (from store.capabilities.rnStyleMeasure) to backward compatibility for the backend vendor.

Can you explain why you can’t call measureStyle directly in the frontend where you display it instead of going through the bridge? (I’m not sure I understand this part of React DevTools so bear with me.) It would be cool if it “just worked” with old RN versions too. You could also remove the 'rn-style:measure' calls then.

The instance.measure is from RN UIManager.measure, it's native method.

OK, this makes sense. You can’t call it because it lives in the RN process.

gaearon · 2017-04-20T17:10:46Z

+    bridge.send('rn-style:measure', {
+      id, style,
+      measureLayout: {
+        x, y,


Please format each property on next line (above too).

gaearon

I’d like to better understand why the measurement code can’t completely move to frontend (and thus work on older RN versions).

gaearon

Some naming change requests.

gaearon · 2017-04-20T17:51:14Z

  // TODO: typecheck bridge interface
  bridge: any;
  id: any;
+  measureSupport: boolean;


Let's call this supportsMeasure.

@gaearon done. :)

gaearon · 2017-04-20T17:51:23Z


 type State = {
  style: ?Object;
+  measureLayout: ?Object;


Let's call this measuredLayout

gaearon · 2017-04-20T17:51:31Z

+
+type StyleResult = {
+  style: Object;
+  measureLayout: ?Object;


Let's call this measuredLayout

gaearon · 2017-04-20T17:51:53Z

+    var padding = (style && resolveBoxStyle('padding', style)) || blank;
+    bridge.send('rn-style:measure', {
+      style,
+      measureLayout: {


measuredLayout

gaearon

When editing styles, changes don’t seem to take effect immediately. I have to switch to another component and back to see the measurements change. Can we fix this?

jhen0409 · 2017-04-21T19:07:31Z

It seems like have an async change in rename and set, so measureStyle will get old layout (apart from margin an padding), just set setTimeout for measureStyle it works fine.

gaearon · 2017-04-25T14:09:43Z

Seems to work well. Thank you!

jhen0409 added 3 commits March 20, 2017 03:23

Add style measure on rn-style:get

f767da6

Add BoxInspector for show measure layout

946a42d

Measure layout after rn-style:rename and rn-style:set

b4dafaa

facebook-github-bot added GH Review: review-needed CLA Signed labels Mar 20, 2017

jhen0409 mentioned this pull request Mar 20, 2017

How to view dimensions (height/width etc.) of an element jhen0409/react-native-debugger#52

Closed

jhen0409 added 2 commits March 20, 2017 20:51

Change event name to rn-style:measure and backward compatibility

3f57927

Should send event if node not found

d3d6e7b

suchipi reviewed Apr 10, 2017

View reviewed changes

Tweak BoxInspector props

969565e

jhen0409 force-pushed the box-inspect branch from cdaf66b to bf06521 Compare April 11, 2017 15:06

jhen0409 added 2 commits April 14, 2017 04:22

Fix style change of NativeStyler

f8c560b

Define _styleGet func type of NativeStyler

e8465e1

jhen0409 force-pushed the box-inspect branch from bf06521 to e8465e1 Compare April 13, 2017 20:24

gaearon reviewed Apr 20, 2017

View reviewed changes

jhen0409 added 2 commits April 20, 2017 23:16

Mirror resolveBoxStyle from react-native

ce3cbde

Fix rn-style:get call argument

d4d9ea7

jhen0409 force-pushed the box-inspect branch from 4a332c2 to d4d9ea7 Compare April 20, 2017 15:16

gaearon reviewed Apr 20, 2017

View reviewed changes

gaearon suggested changes Apr 20, 2017

View reviewed changes

Format rn-style:measure event properties

396630f

gaearon suggested changes Apr 20, 2017

View reviewed changes

Change measure prop / state name

b96b44f

gaearon suggested changes Apr 21, 2017

View reviewed changes

This was referenced Apr 21, 2017

[React Native][Feature Request] Display layout styles in a friendlier format for reading #515

Closed

[ReactNative][Feature Request] Inspect dimensions of native views #330

Closed

Add setTimeout for measureStyle call

00b8bdb

gaearon approved these changes Apr 25, 2017

View reviewed changes

gaearon merged commit dc8f87d into facebook:master Apr 25, 2017

jhen0409 deleted the box-inspect branch May 23, 2017 18:41

Uh oh!

Conversation

jhen0409 commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredly commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhen0409 commented Apr 20, 2017

Uh oh!

gaearon Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

jhen0409 commented Apr 21, 2017

Uh oh!

gaearon commented Apr 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jhen0409 commented Mar 20, 2017 •

edited

Loading

gaearon commented Apr 20, 2017 •

edited

Loading

gaearon Apr 20, 2017 •

edited

Loading

gaearon Apr 20, 2017 •

edited

Loading