Implemented vertical splitting into SplitPane #541

jolyonruss · 2017-02-17T12:39:37Z

I uploaded a video to Youtube to show it in action

In Container.js I did the following:

setup an state value isVertical
added an onResize event listener
update the isVertical when the window width is less than 500
pass an initialHeight value to SplitPane.js

In SplitPane.js I did the following:

setup the new props; isVertical & initialHeight
added height to the local state
update the state when isVertical changes in componentWillReceiveProps
updated the onMove method to handle x & y changes
in the render method switched a few values based on isVertical
added some new vertical specific styles, including a border bottom on LeftPane to make this clear where the split when the split is vertical

In DetailPane.js I did 1 thing:

added a light grey background to the header, so that when the split is vertical there's enough contrast to see where the handle/split is. Chrome Devtools' right/bottom pane has a grey tab at the top which helps with this. When the header was white it was very hard to see where the handle is.

facebook-github-bot · 2017-02-17T12:40:02Z

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 [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

jolyonruss · 2017-02-17T12:42:55Z

Oh, didn't realise there was a CLA, have now signed 👍

facebook-github-bot · 2017-02-17T12:47:30Z

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

gaearon · 2017-02-17T14:51:07Z

frontend/Container.js

+  }
+
+  componentDidMount() {
+    window.addEventListener('resize', (e) => this.handleResize(e), false);


Please throttle this event listener. Something like once every 100ms should be fine.

Yeah, that was why I asked about adding a resize :) wasn't sure if you'd want to throttle or not. 100 feels a little slow, 50 seems comparable to the Devtool's element tab.

gaearon

Looks great! Left a few comments.

gaearon · 2017-02-17T14:52:21Z

frontend/Container.js

+  }
+
+  componentWillUnmount() {
+    window.removeEventListener('resize');


I don't think you're unsubscribing since you're not passing the listener.
Instead, I suggest declaring the listener as a class property:

handleResize = (e) => { // ... }

Then you can window.addEventListener('resize', this.handleResize) and window.removeEventListener('resize', this.handleResize).

I actually prefer this syntax, but have had trouble getting it to work in other people's projects before, a simple fix.

It requires class-properties plugin which we use.

gaearon · 2017-02-17T14:53:32Z

frontend/Container.js

+    super(props);
+
+    this.state = {
+      isVertical: (window.innerWidth < 500),


Let's extract a constant since it's used in two places and we might forget one without the other by mistake.
Alternatively you can make a computeState(window) function and use it in both places.

D'you mean in /agent/consts.js? Or as a local constant? Or somewhere completely different?

Here in the file. Just so two 500s are one.

And just to prove the point it's actually three 500s :)

Have named it IS_VERTICAL_BREAKPOINT

gaearon · 2017-02-17T14:54:38Z

frontend/SplitPane.js


-  onMove(x: number) {
+  componentWillReceiveProps(nextProps: Props) {
+    this.setState({


Looks like there's no need to run this code if nextProps.isVertical hasn't changed.

if (nextProps.isVertical !== this.props.isVertical) { // put your code here }

It is dangerous to depend on this.state when calling setState.

And I think you don't need to. Maybe you can do something like this?

if (nextProps.isVertical && !this.props.isVertical) { this.setState({width: '100%'}); } else if (!nextProps.isVertical && this.props.isVertical) { this.setState({height: '100%'}); }

jolyonruss · 2017-02-17T21:01:29Z

I had a look at the failing tests, there are 9 errors, most are fixed by changing the width/height props to a String, since I'm using 100% now which of course isn't a Number.

It's also saying I need to add an annotation to handleResize(e) should this be Object, Event or SyntheticEvent? Here's a link to the specific line in Travis - https://travis-ci.org/facebook/react-devtools/builds/202606486#L565

gaearon · 2017-02-17T21:32:11Z

Maybe SyntheticUIEvent will work.
http://www.saltycrane.com/blog/2016/06/flow-type-cheat-sheet/#lib/react.js

gaearon · 2017-02-17T21:47:16Z

frontend/Container.js

+  handleResizeTimeout(scope: Container, e: Event) {
+    resizeTimeout = null;
+
+    scope.setState({


Let's just use the property syntax so you can safely use this here.

gaearon · 2017-02-17T21:47:35Z

frontend/Container.js

+  }
+
+  componentWillUnmount() {
+    window.removeEventListener('resize', this.handleResize);


Please also cancel the interval here.

I've added the clearTimeout in here

gaearon · 2017-02-17T21:48:48Z

frontend/Container.js

+  }
+
+  handleResize = (e: Event) => {
+    if (!resizeTimeout) {


React synthetic events get reused. I don't think it's safe to pass them around. Could you read target here and then pass just the target?

Actually isn't target always going to be the window?

Yes, it's always going to be window

gaearon · 2017-02-17T21:49:19Z

frontend/SplitPane.js


-  onMove(x: number) {
+  componentWillReceiveProps(nextProps: Props) {
+    if (nextProps.isVertical !== this.props.isVertical) {


The outer condition is now unnecessary because inner conditions always make it true.

jolyonruss · 2017-02-17T22:03:52Z

Sorry about all the little fixes

jolyonruss · 2017-02-19T19:16:39Z

@gaearon d'you have any idea why this check is failing?

 77:   handleResizeTimeout = () => {
       ^^^^^^^^^^^^^^^^^^^ Unexpected identifier

It's my first time using Flow, so not sure if I've misses something obvious.

Link to Travis - https://travis-ci.org/facebook/react-devtools/builds/203209111#L568

jaredly · 2017-02-20T15:48:15Z

Actually could we circumvent all of this by just using a media query?

jolyonruss · 2017-02-20T16:59:57Z

@jaredly cause we're using an onMove to handle the dragging and need to track different directions based on the the layout horizontal or vertical, I can't quite see how this could work with with just a media query.

Happy to hear suggestions on how this could work and refactor if there's a better way to solve this.

gaearon · 2017-02-20T19:33:41Z

frontend/Container.js

+    if (!resizeTimeout) {
+      resizeTimeout = setTimeout(this.handleResizeTimeout, 50);
+    }
+  }


I think you might need to add semicolon at the end of property initializers. This probably breaks some the (potentially old) Flow version we're using.

handleStuff = () => { // ... }; handleOtherStuff = () => { // ... };

You don't need to do it for regular methods though.

gaearon · 2017-02-20T20:00:15Z

frontend/Container.js

+};
+
+var IS_VERTICAL_BREAKPOINT = 500;
+var resizeTimeout = null;


We should still store it on the instance.
If Flow complains, you need to explcitly declare it:

resizeTimeout?: number;

Just like we declare props and state types inside the class.

The linter didn't like the ?: syntax, so I just set it's initial value to null

gaearon · 2017-02-21T00:09:58Z

frontend/Container.js

    extraTabs: {[key: string]: () => React$Element},
  };
  state: State;
+  resizeTimeout: number = null;


If it's a number it can't be null. number | null or ?number would work though.

gaearon · 2017-02-21T00:10:38Z

I recommend to run Flow locally instead of waiting for CI.

gaearon · 2017-02-24T21:42:09Z

I played with it, and I think there a few UI issues I'd like to see addressed.

By default, as soon as I make the window narrow enough, it collapses like this:

I would expect the panels to start 50/50 or with some other reasonable default split size.

Next, if I resize the split point the way I like it, but later resize the window to be wide and back, it doesn't remember my previous vertical split point. If you do the same in Chrome, you'll notice it remembers the previous setting in the vertical mode. We don't have to persist it, but we should have the same behavior at least within a single session.

Finally, while this may not be a problem of this PR in particular, I noticed that when I resize it to be wider (or taller), the tree view stays the same while the secondary panel grows. I expect the opposite to happen. It's the tree panel that should grow or shrink by default. This is also how Chrome panel works.

I think these are three important improvements, and I'd be excited to merge a PR fixing them.
Let me know if you'd like to keep working on this!

gaearon

Please see feedback above.

jolyonruss · 2017-02-28T11:35:05Z

Thanks for the feedback, sorry it's taken a while to reply, I was on holiday last week.

Firstly, I really appreciate the help with fixing the Flow issues I was seeing.

I'd like to continue working on this and fixing the UI issues you listed above, hopefully it shouldn't take too long and it'll be merge-able by the end of the week.

jaredly · 2017-02-28T16:32:59Z

frontend/SplitPane.js

  moving: boolean,
-  width: number,
+  width: string,
+  height: string,


using toString and then parseInt is definitely a smell here.
You can just have width & height be numbers, and forget about setting them to 100%. Then down in render, you just do e.g. props.isVertical ? '100%' : this.state.width

Thanks for pushing for this, I knew it was a bit of a smell when I was writing it, but my focus was on getting the flow check to pass.

Swapping the type back to a number has simplified the code and made it a lot less smelly

jolyonruss · 2017-03-04T19:23:15Z

@gaearon flow check passes locally and I've fixed the three UI issues you listed above.

I swapped the type of width/height back to a number as per @jaredly's suggestion, which cleaned up the code and means I no long need to check when SplitPane receives new props in componentWillReceiveProps.

gaearon · 2017-03-09T03:37:40Z

This looks great. There’s just one final change I’d like to make.

When I wrote last feedback I haven’t fully thought it through. It was slightly contradictory:

I would expect the panels to start 50/50

cannot also work well with

I expect the opposite to happen. It's the tree panel that should grow or shrink by default.

Instead of 50/50, it should start with the detail pane being of reasonable fixed default width (or height). Right now, if you don’t drag it but resize the window, it keeps being 50/50. But if you touch and slightly move it first, then it stays fixed. I’d like it to stay fixed from the very beginning.

Could you do this?

jolyonruss · 2017-03-14T16:26:01Z

I haven't forgotten about this, just got busy - once I get some time it shouldn't take too long to sort.

gaearon · 2017-03-27T13:24:27Z

Just checking in—do you think you’ll have some time to finish it off?

jolyonruss · 2017-03-28T12:19:08Z

Hi Dan, thanks for the poke, just pushed another commit, flow check still passes.

Let me know if that resizing was what you're after.

gaearon · 2017-04-19T18:08:15Z

This works great. 👍

jolyonruss · 2017-04-20T11:08:22Z

😃

gaearon · 2017-04-20T17:43:49Z

Noticed a small issue: #611.

jolyonruss · 2017-04-20T17:52:49Z

Oh, yeah - that shouldn't be doing that! Will take a look as soon as I can, unless someone else wants to pick it up as a good first bug.

Implemented vertical splitting into SplitPane

33d7ff5

facebook-github-bot added the GH Review: review-needed label Feb 17, 2017

facebook-github-bot added the CLA Signed label Feb 17, 2017

gaearon reviewed Feb 17, 2017

View reviewed changes

gaearon suggested changes Feb 17, 2017

View reviewed changes

Fixed minor issues based on comments in PR

d7e26da

Updated annotations to match params

d555b62

gaearon suggested changes Feb 17, 2017

View reviewed changes

A few more minor fixes

a03ddd2

Fixing failing tests for type incompatibility

f6deaac

gaearon reviewed Feb 20, 2017

View reviewed changes

Added semicolon to try and quiet the linter

ee34dff

gaearon suggested changes Feb 20, 2017

View reviewed changes

Moved resizeTimeout to be class property

ccfc3e3

gaearon reviewed Feb 21, 2017

View reviewed changes

gaearon and others added 2 commits February 24, 2017 21:33

Merge branch 'master' into master

69fdf5d

Fix Flow

3d25e60

gaearon approved these changes Feb 24, 2017

View reviewed changes

gaearon suggested changes Feb 24, 2017

View reviewed changes

jaredly suggested changes Feb 28, 2017

View reviewed changes

Swapped type back to number and remembers spilt position

9c14783

jaredly approved these changes Mar 9, 2017

View reviewed changes

SplitPane.js now has a more fixed behaviour when resizing

4a47733

gaearon mentioned this pull request Apr 10, 2017

Bugfix: Correctly calculate Overlay position when inspecting a node within iframe(s) #552

Merged

gaearon approved these changes Apr 19, 2017

View reviewed changes

gaearon merged commit 4ebadbf into facebook:master Apr 19, 2017

gaearon mentioned this pull request Apr 20, 2017

DevTools splitting slightly "jumps" on resizing #611

Closed

gaearon mentioned this pull request Apr 21, 2017

Improve SplitPane.js to support rendering vertical splitting #532

Closed

Implemented vertical splitting into SplitPane #541

Implemented vertical splitting into SplitPane #541

Uh oh!

Conversation

jolyonruss commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Feb 17, 2017

Uh oh!

jolyonruss commented Feb 17, 2017

Uh oh!

facebook-github-bot commented Feb 17, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolyonruss commented Feb 17, 2017

Uh oh!

gaearon commented Feb 17, 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!

jolyonruss commented Feb 17, 2017

Uh oh!

jolyonruss commented Feb 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaredly commented Feb 20, 2017

Uh oh!

jolyonruss commented Feb 20, 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!

gaearon commented Feb 21, 2017

Uh oh!

gaearon commented Feb 24, 2017

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

jolyonruss commented Feb 17, 2017 •

edited

Loading

jolyonruss commented Feb 19, 2017 •

edited

Loading

jolyonruss commented Apr 20, 2017 via email •

edited

Loading