Skip to content

Port view container logic from activitybarpart to panelpart #87444

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

Merged
merged 3 commits into from
Dec 20, 2019
Merged

Port view container logic from activitybarpart to panelpart #87444

merged 3 commits into from
Dec 20, 2019

Conversation

sbatten
Copy link
Member

@sbatten sbatten commented Dec 19, 2019

Refs #85164

I can drop the outline contrib changes, they just make it easy to test.

@sbatten sbatten requested a review from sandy081 December 19, 2019 22:34
@sbatten sbatten self-assigned this Dec 19, 2019
@sbatten sbatten added this to the December/January 2020 milestone Dec 19, 2019
Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved given the provided comments are taken care of

@@ -448,6 +582,11 @@ export class PanelPart extends CompositePart<Panel> implements IPanelService {
this.storageService.store(PanelPart.PINNED_PANELS, value, StorageScope.GLOBAL);
}

private getViewContainer(panelId: string): ViewContainer | undefined {
const viewContainerRegistry = Registry.as<IViewContainersRegistry>(ViewContainerExtensions.ViewContainersRegistry);
return viewContainerRegistry.get(panelId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we have to look for the view container only in the Panel location? (even though ids are unique).

}

private onPanelOpen(panel: IPanel): void {
this.activePanelContextKey.set(panel.getId());

// This panel supports view containers
if (panel instanceof PaneCompositePanel) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would check if view container exists/registered instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked activityBarPart and I think we do not need to have this check and we can do it for all panels.

this.compositeBar.activateComposite(panel.getId());

// This panel supports view containers
if (panel instanceof PaneCompositePanel) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. You can remove this check because you check for view container after


return cachedPanel?.views && cachedPanel.views.length
? cachedPanel.views.every(({ when }) => !!when && !this.contextKeyService.contextMatchesRules(ContextKeyExpr.deserialize(when)))
: panelId === TEST_VIEW_CONTAINER_ID /* Hide Test Panel for the first time or it had no views registered before */;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking with TEST_VIEW_CONTAINER_ID is specific to Sidebar, so not sure if you need this here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

originally left this in if we didn't enforce unique ids across location and we chose to re-use the test container, removing.

@sbatten sbatten merged commit 57c30dd into microsoft:master Dec 20, 2019
@sbatten sbatten deleted the step2 branch December 20, 2019 16:22
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants