Drag and drop layout management #252
Conversation
|
Could you add specs for this so that we are sure that we don't break it in future changes? |
|
Sure thing, I've added a couple of tests and will finish them off tomorrow – let me know if they look ok so far. |
|
👍 Looks really good so far! Please @-mention me when you've got things fixed up so I can review it again. Thanks again for the hard work! |
|
@lee-dohm Glad I can be helpful – and thank you for the review! I've finished off the last couple tests. |
|
Just bumping this, would be great to get a review. By the way, for the testing I've had to inject test values into the layout module in place of the usual |
|
Thanks, it's in our review queue. Our review queue, however, is long, so it might take a bit to get to. |
|
Ok great, well it's good to know it's in the pipeline, so thanks for the update! |
|
I'm not a keyboard guy and applaud any UI improvements, but this is super awesome! 👍 I've played with it and it works great. Did notice that it only works when the tab is focused, but that could be an original tabs issue. Was wondering about your cursor in the screengrab... I'm on Windows and there it shows just the drag cursor ( Atom 1.5.0-beta3 |
|
|
||
| innerBounds: ({left, top, width, height}, [x, y, w, h]) -> | ||
| left += x*width; top += y*height | ||
| width *= w; height *= h |
There was a problem hiding this comment.
Intentional use of semicolon?
There was a problem hiding this comment.
Yup, I just found that laying the statements out in a rectangle made it more intuitive for me. I'll be happy to defer to the maintainers if people don't like the style, of course.
There was a problem hiding this comment.
Yeah, it would be great to get this all on separate lines 😄 Thanks!
|
@jerone thanks for the feedback, and glad you like it! I haven't (intentionally) changed the cursor or focus behaviour, but did also find it odd that it only works with a focused |
|
There's something strange going on with the tests – they passed fine before but now fail despite no code changes (the empty commit |
|
The production version of Atom changed from v1.4.3 to v1.5.0 yesterday which included some pending tabs changes, I believe. This PR will need to be updated to work with that feature. (Fortunately, it is only one failing test 😀) |
|
Looks like it's an issue with the master branch of this package, rather than being specific to this PR. |
|
Have rebased over the now-working master and the green light is back on. |
|
This is really amazing! Great work @MikeInnes 🎉 ❤️ |
|
Can't wait to see this merged!! 👍 |
|
@MikeInnes mentioned a problem, quoted below. Issue #190 is tracking this, I just wanted to link them.
|
|
Just bumping this again, I don't want to be impatient but it has been nearly four months since I opened the PR. |
|
👍 on adding this. It feels pretty intuitive and quick without having to think what command to use. |
|
@MikeInnes Would you mind fixing the merge conflicts? Once you do, I'll take a look at getting this merged. |
| activate: -> | ||
| @view = document.createElement 'div' | ||
| @view.classList.add 'layout-overlay' | ||
| document.body.appendChild @view |
There was a problem hiding this comment.
Not too critical, but maybe something to consider. Move the layout-overlay element inside atom-pane-container.panes?
Just so the DOM root stays a bit more organized. Not sure if this would mess up the calculations.
And/or rename it to tabs-layout-overlay. Then it's more clear that this element is part of the tabs package and shouldn't be used for something else.
There was a problem hiding this comment.
Thanks for the tips, will go ahead and do both!
There was a problem hiding this comment.
Ok so I did the rename and played around with adding the overlay to atom-pane-container.panes. Doing that complicates the layout logic a little (which should be fixable) but unfortunately also seems to confuse Atom's pane logic, resulting in empty panes appearing and such.
Instead I added the overlay to atom-workspace, which which I think solves the issue of keeping the root node clean – hope that's ok.
|
@lee-dohm will do, thanks a lot for taking a look. |
|
Thanks so much for all your hard work on this @MikeInnes! |
|
This is Right and Good, 👏 👏 : clap: |
|
Awesome, very happy to see this in – thank you @lee-dohm! |
|
Amazing! I made my own plugin for this behavior, tabasco, but I'll rather use the official implementation once it's part of Atom. Very nice! |
|
Damn. Nice. 👊🏼 |
|
This is one of my favorite features of 2016. Love using it. Any chance of making it work when dropping the tab in a new window? atom/atom#14273 |
This kind of UI is really handy for those who are less keyboard-savvy, especially if you have a bunch of plots / workspace panes / consoles etc. open and don't want to remember the right sequence of commands needed to organise things :)
I'm still fairly new to CoffeeScript / Atom development so feedback is definitely appreciated. The global
@lastSplitapproach is a bit weird, but seems to be necessary as thedragendevent gives differentclientXandclientYcoordinates which throws things off. I've aimed for an overlay styling that works well with both light and dark syntax / ui themes, but other suggestions are appreciated there as well.