add implementation for resizable panes, just solve the issue #274 #5902

liuxiong332 · 2015-03-09T06:26:35Z

this commit request implement resizable panes and achieve the feature of #274.

printscreen:

approach

the approach is inserting resizable view (named pane-resize-handle-view) between two panes. when user move the resizable view, I will change the pane's flexGrow style value by two pane's width or height ratio. for example, if the left pane is 200px and the right pane is 600px, then I will modify flexGrow style to 0.5 and 1.5.
This way is like tree view's resizable view.

…ment

…evious and next pane will change.

…fault open url link. add listener for pane's drop event to open dragged files.

benogle · 2015-03-09T17:20:46Z

src/pane-resize-handle-view.coffee

@@ -0,0 +1,67 @@
+{$, View} = require 'atom-space-pen-views'


Can you remove all references of view and jQuery? We're no longer using space pen or jQuery in core. In place of jQuery, please use the raw DOM APIs.

benogle · 2015-03-09T17:23:25Z

This looks interesting! A couple things before we can merge:

Can you please write some specs?
Please remove all references to jQuery and View

…h PaneResizeHandleElement

liuxiong332 · 2015-03-10T13:46:08Z

I have remove all references to jQeury and View @benogle and add specs for pane axis element to make sure the insertion logic of resizable view is correct.
But, I have no idea how to write specs for resizable element. Maybe the good test way is the UI test using some tools such as chrome driver, but now it cannot achieve.

philipgiuliani · 2015-03-10T14:38:10Z

Really cool! Thanks for this :) Finally split-views make sense! 👏

philipgiuliani · 2015-03-10T21:47:11Z

src/pane-element.coffee

+      event.preventDefault()
+      event.stopPropagation()
+      @getModel().activate()
+      pathsToOpen = _.pluck(event.dataTransfer.files, 'path')


You could write the following i think, so you could remove the dependency to _ (i just saw they are trying that in new packages currently)...

pathsToOpen = event.dataTransfer.files.map (file) -> file.path

See array.map

Update:
I just saw that dataTransfer.files returns a FileList, so array.map won't be available unless you convert it to one :(

Now I have removed the dependency to _ and replaced with Array::map.

pathsToOpen = Array::map.call event.dataTransfer.files, (file) -> file.path

Thank you.

Did you push this change? I'm still seeing _ being used at the head of your branch.

He removed it in the second PR #5910

I really would like to merge this stuff, but I need a single consolidated PR with good test coverage first. Can someone else from the community collaborate on the test side of things?

rmartin · 2015-03-13T19:13:41Z

hey @liuxiong332 ,

I had a chance to take a look at this today and overall this looks great. The approach I had taken was to integrate this within the resize-pane package, but that had several challenges when purely relying on the callbacks. I like the approach you took here of adding this to core and making the necessary changes to the Pane Axis to adjust the children. I also liked a few additions that I hadn't considered like double-click to balance the frames.

Overall this looks like a great fix and if this gets integrated into core then I can just shelve the work I was doing on the other package. Looking through your repo, it looks like you've also updated but not yet committed the resize-pane package to accommodated for this new approach, which is great.

I just had a few comments on my end based on my experience working on that the last few weeks, but nothing that should prevent moving forward with this.

Resize Pane Width - When doing research on determining what size this should be, I found Chris Coyier's approach helpful (http://codepen.io/chriscoyier/blog/design-is-hard). My suggestion would be to follow suite there with the height / width of the resize panes and adjust them from the current 8px down to 6px. This is an opinion more than anything but after playing around with them for a while this seemed like the 'right' size.
Rebalance Panes After Close - One thing I noted in your solution, that I also struggled with, is that after you resize a pane it 'retains' the flex width value. You took the similar approach that I did by clearing this out for the last element. One suggestion I would have is to 'rebalance' the panes when one of the panes close. This is also likely a minor item that can be improved in the future. Here is one scenario to see this would be to do the following:
- Open a single editor, split right, split right. So you have three total panes.
- resize the first pane so that it's smaller.
- close the second and third pane
- split right the first pane. Notice that because the first pane still has a flexbox width that the pane is not 50/50 width instead it retained the old value.
Spec Tests - One thing to add here would be to actually test a few different scenarios by splitting the panes. That way you can simulate the user scenarios of splitting the pane. Although it needed to be refactored, you can see this in action within this spec test. https://github.com/rmartin/resize-panes/blob/issue-7-resize-with-mouse/spec/resize-pane-view-spec.coffee

In either case, glad to see that this is getting added and if there is anything that I can do to help get that merged into core let me know. Overall great work.

Cheers,
Roy

nathansobo · 2015-03-16T18:23:59Z

@simurai Can you weigh in on the design aspects of this? Note the thickened draggable border that's added between panes.

@liuxiong332 The updated pane size isn't being serialized down into the model, so it gets blown away on refresh. Could you explore adding that? We really want refresh to be a first-class concept so the view state is retained as much as possible when people do it.

nathansobo · 2015-03-16T18:26:27Z

spec/pane-axis-element-spec.coffee

+      createdCallback: ->
+        @classList.add('test-root')
+
+    TestItemElement = document.registerElement 'atom-test-item-element', prototype: TestItemElement.prototype


I think you should be able to use a generic div element to test this rather than registering your own. Adding a custom element here adds complexity and it also can never be registered with the current custom elements API.

- Keeps most themes unaffected - More consistent with the tree-view resizer

simurai · 2015-03-17T07:35:14Z

Note the thickened draggable border that's added between panes.

Wouldn't be a huge issue, but to keep most themes visually untouched, I made a PR on top of this one: liuxiong332#1 It removes the "thickened draggable border" and instead the atom-pane-resize-handle overlap the editors on each side a bit. Themes could still override it if the space between fits better.

…flex scale

philipgiuliani · 2015-03-17T07:51:59Z

Looks really good @simurai :) Already happy to get this one merged!

absolute positions the pane resize-handles and overlap the editor

liuxiong332 · 2015-03-17T11:35:18Z

Looks good @simurai . Thank you!

pitaj · 2015-03-18T01:11:44Z

I implemented something like this as a temporary fix before this is merged

https://github.com/pitaj/atom-resize-panes/

…est the resize behavior.

winstliu · 2015-04-15T11:45:38Z

spec/pane-container-element-spec.coffee

+      leftPane.close()
+      expectPaneScale [rightPane, 1]
+
+    it "split or close panes in orthogonal direction", ->


splits or closes adjacent panes

Again, not sure if this is accurate

I just want to test that the scale of horizontal panes is not changed when the vertical panes are added or removed.

nathansobo · 2015-04-15T13:07:28Z

/cc @maxbrunsfeld

maxbrunsfeld · 2015-04-15T20:31:29Z

Thanks a lot @liuxiong332. A lot of people will be excited to have this feature.

add implementation for resizable panes, just solve the issue #274

philipgiuliani · 2015-04-15T20:40:44Z

Whaaat its mergeeed 💥 Thanks @liuxiong332 for working on this!! :)

thomasjo · 2015-04-15T20:43:26Z

Awesome ⚡❗

benogle · 2015-04-15T21:13:44Z

🎉 🎉 Pulling and building...

rmartin · 2015-04-15T21:29:18Z

Yahoo! well done @liuxiong332

liuxiong332 · 2015-04-16T01:11:00Z

Awesome!!

jordiorlando · 2015-04-16T04:02:11Z

Wow, fantastic work @liuxiong332!

simurai · 2015-04-16T08:07:36Z

😻

mikemcbride · 2015-04-16T15:19:11Z

👏 so pumped about this. awesome work @liuxiong332. thank you!

atticoos · 2015-04-22T01:45:42Z

👏 thanks @liuxiong332!

trkoch · 2015-04-22T07:44:28Z

Thanks @liuxiong332! One minor issue: The resize icon is not the same that appears when resizing the file drawer.

nathansobo · 2015-04-22T17:37:18Z

One minor issue: The resize icon is not the same that appears when resizing the file drawer.

/cc @simurai on this slight inconsistency.

chrishough · 2015-06-05T19:59:59Z

I noticed this recently broke in the latest version of atom. I can no longer use cmd+alt-- to shrink or expand windows, thoughts?

mmcintyre123 · 2015-07-14T16:08:52Z

Can you add this to atom in ubuntu? I'm running on ubuntu 14.04 LTS and atom 1.0.2 and none of the panes are resizable. Also commented on #274. Thanks!

liuxiong332 added 3 commits March 9, 2015 13:16

add pane-resize-handle-view and insert resize view into pane axis ele…

651139e

…ment

fix the bug that when user dynamic add panes, the resizable view's pr…

026eb40

…evious and next pane will change.

invalidate document's drag and drop event to prevent the browser's de…

71a2797

…fault open url link. add listener for pane's drop event to open dragged files.

benogle reviewed Mar 9, 2015
View reviewed changes

liuxiong332 added 2 commits March 10, 2015 17:02

delete PaneResizeHandleView that use jQuery and View and replaced wit…

f8f4226

…h PaneResizeHandleElement

add specs for pane axis element

9b77d30

philipgiuliani reviewed Mar 10, 2015
View reviewed changes

benogle assigned nathansobo Mar 11, 2015

nathansobo reviewed Mar 16, 2015
View reviewed changes

Absolute position pane resize-handle

4980ce3

- Keeps most themes unaffected - More consistent with the tree-view resizer

simurai mentioned this pull request Mar 17, 2015

Resizable Panes liuxiong332/atom#1

Merged

liuxiong332 added 2 commits March 17, 2015 15:35

add serialization for pane's flex scale when resize the pane

c05a410

when split panes and replace the pane with new pane axis, switch the …

86ef14b

…flex scale

liuxiong332 added 2 commits March 17, 2015 17:25

adjust the flex scale when close pane

c9c050d

Merge pull request #1 from atom/sm-pane-resize

3b0b2f3

absolute positions the pane resize-handles and overlap the editor

liuxiong332 added 4 commits March 18, 2015 13:33

simplify the pane-axis-element-spec spec files

60434b1

replace 'fdescribe' with 'describe' in pane-axis-element-spec.coffee

8b13771

add specs which simulate mouse action in PaneResizeHandleElement to t…

4774276

…est the resize behavior.

delete some not used statement

e54c5b0

winstliu reviewed Apr 15, 2015
View reviewed changes

change the spec description to make it clear

1331cfa

maxbrunsfeld pushed a commit that referenced this pull request Apr 15, 2015

Merge pull request #5902 from liuxiong332/pane-resize

632c57d

add implementation for resizable panes, just solve the issue #274

maxbrunsfeld merged commit 632c57d into atom:master Apr 15, 2015

liuxiong332 mentioned this pull request Apr 16, 2015

this commit make Atom open files in the pane view that files was dropped into #5910

Closed

philipgiuliani mentioned this pull request Apr 16, 2015

Allow panes to be resized #274

Closed

liuxiong332 deleted the pane-resize branch April 16, 2015 08:50

This was referenced Jun 9, 2015

Resize pane cursor is inconsistent with tree view cursor #7178

Closed

Fix inconsistency with pane resize cursors. #7179

Merged

This was referenced Jun 17, 2015

Open drag&dropped file in correct split view #4049

Closed

Broken in latest Atom Release santip/resize-panes#21

Closed

izuzak mentioned this pull request Jun 19, 2015

Add Resize - Panel Keyboard Commands #7332

Closed

dcalhoun mentioned this pull request Jun 29, 2015

Package not found in Atom search results santip/resize-panes#22

Closed

brumm mentioned this pull request Dec 5, 2015

Initial size could be specified in px (not just in %) nathancahill/split#10

Closed

Arcanemagus removed the needs-review label Mar 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add implementation for resizable panes, just solve the issue #274 #5902

add implementation for resizable panes, just solve the issue #274 #5902

liuxiong332 commented Mar 9, 2015

benogle Mar 9, 2015

benogle commented Mar 9, 2015

liuxiong332 commented Mar 10, 2015

philipgiuliani commented Mar 10, 2015

philipgiuliani Mar 10, 2015

liuxiong332 Mar 11, 2015

nathansobo Mar 16, 2015

philipgiuliani Mar 16, 2015

nathansobo Mar 16, 2015

rmartin commented Mar 13, 2015

nathansobo commented Mar 16, 2015

nathansobo Mar 16, 2015

simurai commented Mar 17, 2015

philipgiuliani commented Mar 17, 2015

liuxiong332 commented Mar 17, 2015

pitaj commented Mar 18, 2015

winstliu Apr 15, 2015

liuxiong332 Apr 15, 2015

nathansobo commented Apr 15, 2015

maxbrunsfeld commented Apr 15, 2015

philipgiuliani commented Apr 15, 2015

thomasjo commented Apr 15, 2015

benogle commented Apr 15, 2015

rmartin commented Apr 15, 2015

liuxiong332 commented Apr 16, 2015

jordiorlando commented Apr 16, 2015

simurai commented Apr 16, 2015

mikemcbride commented Apr 16, 2015

atticoos commented Apr 22, 2015

trkoch commented Apr 22, 2015

nathansobo commented Apr 22, 2015

chrishough commented Jun 5, 2015

mmcintyre123 commented Jul 14, 2015

add implementation for resizable panes, just solve the issue #274 #5902

add implementation for resizable panes, just solve the issue #274 #5902

Conversation

liuxiong332 commented Mar 9, 2015

this commit request implement resizable panes and achieve the feature of #274.

approach

Choose a reason for hiding this comment

benogle commented Mar 9, 2015

liuxiong332 commented Mar 10, 2015

philipgiuliani commented Mar 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmartin commented Mar 13, 2015

nathansobo commented Mar 16, 2015

Choose a reason for hiding this comment

simurai commented Mar 17, 2015

philipgiuliani commented Mar 17, 2015

liuxiong332 commented Mar 17, 2015

pitaj commented Mar 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathansobo commented Apr 15, 2015

maxbrunsfeld commented Apr 15, 2015

philipgiuliani commented Apr 15, 2015

thomasjo commented Apr 15, 2015

benogle commented Apr 15, 2015

rmartin commented Apr 15, 2015

liuxiong332 commented Apr 16, 2015

jordiorlando commented Apr 16, 2015

simurai commented Apr 16, 2015

mikemcbride commented Apr 16, 2015

atticoos commented Apr 22, 2015

trkoch commented Apr 22, 2015

nathansobo commented Apr 22, 2015

chrishough commented Jun 5, 2015

mmcintyre123 commented Jul 14, 2015