-
Notifications
You must be signed in to change notification settings - Fork 30.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enabled movable Custom Views #89240
Enabled movable Custom Views #89240
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.
Reviewed changes in ViewDescriptorService
. Requested one change.
Approving it expecting that requested change is taken care of.
Thanks.
@@ -230,7 +236,7 @@ export class ViewDescriptorService extends Disposable implements IViewDescriptor | |||
this._register(this.viewsRegistry.onViewsRegistered(({ views, viewContainer }) => this.onDidRegisterViews(views, viewContainer))); | |||
this._register(this.viewsRegistry.onViewsDeregistered(({ views, viewContainer }) => this.onDidDeregisterViews(views, viewContainer))); | |||
|
|||
this._register(this.viewsRegistry.onDidChangeContainer(({ views, from, to }) => { this.removeViews(from, views); this.addViews(to, views); })); | |||
this._register(this.viewsRegistry.onDidChangeContainer(({ views, from, to }) => { this.removeViews(from, views); this.addViews(to, views); this._onDidChangeContainer.fire({ views, from, to }); })); |
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 think we shall also check location change here even though currently location change is not yet supported in registry. This should be ready for future if it is allowed.
I would recommend to have a single method that can be reused when views are moved in registry or in cache.
this.removeViews(from, views);
this.addViews(to, views);
const oldLocation = this.viewContainersRegistry.getViewContainerLocation(from)!;
const newLocation = this.viewContainersRegistry.getViewContainerLocation(to)!;
if (oldLocation !== newLocation) {
this._onDidChangeLocation.fire({ views, from: oldLocation, to: newLocation });
}
this._onDidChangeContainer.fire({ views, from, to });
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.
@sandy081 thanks, will take care of this.
It would still be great to get approval from: |
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.
Changes in listService
make sense!
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.
The custom view changes look good!
@sbatten Merging the PR as we have all approvals. |
This PR refs #85164