Conversation
|
@isidorn cool, I guess something is off with the background color?
There is also a bit weirdness when running this when no editor is opened:
|
|
Also I would not mind if the centered widget was always used and when centered is off it would just fill the whole space and not allow to drag the sashes and when enabled it toggles. That way we do not need to perform any reparenting when switching modes. |
|
@isidorn I would merge |
|
I have merged master. Still needs some polish from my first comment, once I address that I plan to merge this in if nobody objects |
| // Layout Grid | ||
| try { | ||
| this.gridWidget.layout(this.dimension.width, this.dimension.height); | ||
| if (this.centeredViewLayout) { |
There was a problem hiding this comment.
Isn't there always a centeredViewLayout? This will always be true, it seems.
| this.doCreateGridControl(); | ||
| this.centeredViewLayout = new CenteredViewLayout(this.container, { | ||
| element: this.gridWidget.element, | ||
| layout: size => this.gridWidget.layout(size, this.dimension ? this.dimension.height : this.gridWidget.maximumHeight), |
There was a problem hiding this comment.
This looks super weird. There must always be a this.dimension. It's impossible for this view to be asked for layout before doLayout has run.
Also, if you want to remove the weirdness, don't simply use IView from splitview. Use IView from gridview. This one talks about width and height. Simultaneously, CenteredViewLayout.layout would take in both width and height.
| } | ||
|
|
||
| activate(active: boolean): void { | ||
| this.active = active; |
There was a problem hiding this comment.
You probably also need if (this.active === active) { return; } before this.
| }; | ||
|
|
||
| this.splitView.addView(emptyView, Sizing.Distribute, 0); | ||
| this.splitView.addView(emptyView, Sizing.Distribute); |
There was a problem hiding this comment.
You need to add two different views... How can that DOM element be put twice in the DOM?
| this.active = active; | ||
| if (this.active) { | ||
| const emptyView = { | ||
| element: $('.'), |
There was a problem hiding this comment.
$('.') doesn't mean anything. If you wan a div, use $('div'). Though you probably should use something descriptive like $('.centered-layout-margin').
| this.splitView.addView(emptyView, Sizing.Distribute); | ||
| } else { | ||
| this.splitView.removeView(0); | ||
| this.splitView.removeView(1); |
There was a problem hiding this comment.
Why not disposing the splitview altogether and just put the provided view directly as a child of the container?
| } | ||
| if (this.centeredViewLayout) { | ||
| this.centeredViewLayout = dispose(this.centeredViewLayout); | ||
| } |
There was a problem hiding this comment.
All you need is this.centeredViewLayout.dispose().
|
@joaomoreno thanks for the review, I have addressed your feedback. There are still some nit-bits to take care of which I am looking into now. What I find a bit ugly is that for some methods the |
| element: this.view.element, | ||
| maximumSize: this.view.maximumWidth, | ||
| minimumSize: this.view.minimumWidth, | ||
| onDidChange: Event.None, |
There was a problem hiding this comment.
You should expose this.view.onDidChange here.
| this.view = view; | ||
| if (this.splitView) { | ||
| this.splitView.removeView(1); | ||
| this.splitView.addView(this.getView(), Sizing.Distribute, 1); |
There was a problem hiding this comment.
There is splitview.distributeViewSizes().
| if (this.active) { | ||
| const emptyView = { | ||
| element: $('.'), | ||
| if (active) { |
There was a problem hiding this comment.
You should still make a if (active === !!this.splitview) { return; }, so you don't get any NPEs.
|
@joaomoreno thanks - great feedback, I have addressed that feedback in the latest commit. |
Fixes #50936
Brings back centered editor layout support to VSCode.
This is work in progress, creating PR now to potentially get early feedback.
Still needs to be done: