-
Notifications
You must be signed in to change notification settings - Fork 166
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review of all but the main dock panel file.
@@ -321,6 +330,8 @@ function main(): void { | |||
|
|||
Widget.attach(bar, document.body); | |||
Widget.attach(main, document.body); | |||
|
|||
(window as any).dock = dock; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentionally left in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -617,6 +610,7 @@ class SplitLayout extends PanelLayout { | |||
if (this.parent) this.parent.update(); | |||
} | |||
|
|||
// TODO rename this to `moveHandle` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these TODOs intended for another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
@@ -655,21 +649,26 @@ class SplitLayout extends PanelLayout { | |||
|
|||
// Prevent widget resizing unless needed. | |||
each(this._sizers, sizer => { | |||
// TODO is this check actually necessary? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
??
* | ||
* This signal is emitted even if the clicked tab is the current tab. | ||
*/ | ||
tabActivateRequested: ISignal<TabBar, TabBar.ITabActivateRequestedArgs>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this signal also contained the original event (or at least the button and the client position), we could use it for #104.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to avoid emitting events as signals. The signal should have semantic meaning, rather than eventful meaning.
protected init(): void { | ||
super.init(); | ||
let index = 0; | ||
each(this, widget => { this.attachWidget(index++, widget); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you choose not to use enumerate
here? Performance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
expect(layout.removeWidgetAt(0)).to.be(widget); | ||
let layout = new LogPanelLayout(); | ||
widget.layout = layout; | ||
expect(layout.methods.indexOf('init')).to.not.be(-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(layout.methods).to.contain('init');
reads a little nicer.
let widgets = [new LogWidget(), new LogWidget(), new LogWidget()]; | ||
each(widgets, w => { layout.addWidget(w); }); | ||
parent.layout = layout; | ||
expect(layout.methods.indexOf('init')).to.not.be(-1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto for this one and a few more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will save test cleanup for another PR
I was just getting used to this. VS Code is nice in that it lets you either drag to a tab bar or drag to the middle. I've noticed it feels quite a bit faster to drag to the middle (once I got used to it...) |
The API is largely the same.
Summary of breaking changes:
layout-changed
message has been replaced by an explicit call tolayout.init()
.EmptyIterator.instance
is replaced withempty()
for consistency with other iterators.deactivate-request
message and related methods/handlers.removeWidget
an abstract method onLayout
.showOverlay
andfindDropTarget
methods ofDockPanel
are nowprivate
.zone
property of theIOverlayGeometry
interface has been removed.Summary of additions:
once
,repeat
, andchain
.contains
iterator search function.tabActivateRequested
signal toTabBar
.DockLayout
class.DockPanel
now accepts anIRenderer
for creating tab bars and handles.DockPanel
for creating iterators over widgets, tab bars, and handles.Summary of behavioral changes:
Summary of style-related changes:
DockPanel
is now flat. It has four subcontrols:p-DockPanel-tabBar
,p-DockPanel-widget
,p-DockPanel-handle
, andp-DockPanel-overlay
.p-DockPanel-tabPanel
andp-DockPanel-splitPanel
are gone.