Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove the 馃 since we have modes in the go to file #164437

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Remove the 馃 since we have modes in the go to file #164437

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/vs/workbench/browser/layout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,11 @@ export abstract class Layout extends Disposable implements IWorkbenchLayoutServi
}
if (this.isVisible(Parts.TITLEBAR_PART)) {
top += this.getPart(Parts.TITLEBAR_PART).maximumHeight;
quickPickTop = this.titleService.isCommandCenterVisible ? quickPickTop : top;
quickPickTop = top;
}
// If the command center is visible then the quickinput should go over the title bar and the banner
if (this.titleService.isCommandCenterVisible) {
quickPickTop = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change? Will also overlay the banner which I believe is unwanted?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrieken I actually wanted that. because otherwise the command center disappears and it looks weird.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense to my but maybe /cc @daviddossett given emotion with the banner were high. I agree for better looks but I believe the point of the banner is to not be permanent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I follow鈥攚hat is the effect of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@daviddossett so you know we have this banner:
image

today, if you have that banner (be in an untrusted workspace) and you launch the command center this happens:
image
Actually I think that's a bug because really what the code was intending to do, originally, was have the quick pick be under the banner so it didn't cover it basically with the idea that "overlaying anything over the banner is bad"...

My change says "covering the banner for the quick pick is ok" and has the quick pick do what it does today in an untrusted workspace.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think covering it is totally ok personally. It would be strange to decouple the command center button/input from the quick pick itself.

I don't actually see the titlebar element in the second screenshot. Is that intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it is hidden when you click on it since it is often longer than the quickpick itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, got it. In any case I think quick picks should always take priority given their z-index and not be considered part of the normal flow of elements. In other words, they should always be positioned at the very top of the window IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we're all in agreement. I'll defer to @jrieken for a re-review

}
return { top, quickPickTop };
}
Expand Down
36 changes: 4 additions & 32 deletions src/vs/workbench/browser/parts/titlebar/commandCenterControl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { EventLike, reset } from 'vs/base/browser/dom';
import { reset } from 'vs/base/browser/dom';
import { BaseActionViewItem, IBaseActionViewItemOptions } from 'vs/base/browser/ui/actionbar/actionViewItems';
import { IHoverDelegate } from 'vs/base/browser/ui/iconLabel/iconHoverDelegate';
import { setupCustomHover } from 'vs/base/browser/ui/iconLabel/iconLabelHover';
Expand Down Expand Up @@ -62,9 +62,6 @@ export class CommandCenterControl {
super.render(container);
container.classList.add('command-center');

const left = document.createElement('span');
left.classList.add('left');

// icon (search)
const searchIcon = renderIcon(Codicon.search);
searchIcon.classList.add('search-icon');
Expand All @@ -74,22 +71,13 @@ export class CommandCenterControl {
const labelElement = document.createElement('span');
labelElement.classList.add('search-label');
labelElement.innerText = label;
reset(left, searchIcon, labelElement);

// icon (dropdown)
const right = document.createElement('span');
right.classList.add('right');
const dropIcon = renderIcon(Codicon.chevronDown);
reset(right, dropIcon);
reset(container, left, right);
reset(container, searchIcon, labelElement);

// hovers
this._store.add(setupCustomHover(hoverDelegate, right, localize('all', "Show Search Modes...")));
const leftHover = this._store.add(setupCustomHover(hoverDelegate, left, this.getTooltip()));
const hover = this._store.add(setupCustomHover(hoverDelegate, container, this.getTooltip()));

// update label & tooltip when window title changes
this._store.add(windowTitle.onDidChange(() => {
leftHover.update(this.getTooltip());
hover.update(this.getTooltip());
labelElement.innerText = this._getLabel();
}));
}
Expand Down Expand Up @@ -119,22 +107,6 @@ export class CommandCenterControl {

return title;
}

override onClick(event: EventLike, preserveFocus = false): void {

if (event instanceof MouseEvent) {
let el = event.target;
while (el instanceof HTMLElement) {
if (el.classList.contains('right')) {
quickInputService.quickAccess.show('?');
return;
}
el = el.parentElement;
}
}

super.onClick(event, preserveFocus);
}
}

return instantiationService.createInstance(CommandCenterViewItem, action, {});
Expand Down
23 changes: 3 additions & 20 deletions src/vs/workbench/browser/parts/titlebar/media/titlebarpart.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,35 +134,18 @@
border-color: var(--vscode-commandCenter-inactiveBorder) !important;
}

.monaco-workbench .part.titlebar>.titlebar-container>.window-title>.command-center .action-item.command-center .left {
display: inline-flex;
justify-content: center;
width: 100%;
margin: auto;
overflow: hidden;
text-overflow: ellipsis;
}

.monaco-workbench .part.titlebar>.titlebar-container>.window-title>.command-center .action-item.command-center .left .search-icon {
.monaco-workbench .part.titlebar>.titlebar-container>.window-title>.command-center .action-item.command-center .search-icon {
font-size: 14px;
opacity: .8;
margin: auto 3px;
}

.monaco-workbench .part.titlebar>.titlebar-container>.window-title>.command-center .action-item.command-center .left .search-label {
.monaco-workbench .part.titlebar>.titlebar-container>.window-title>.command-center .action-item.command-center .search-label {
overflow: hidden;
text-overflow: ellipsis;
}

.monaco-workbench .part.titlebar>.titlebar-container>.window-title>.command-center .action-item.command-center .right {
margin-left: auto;
padding: 2px 2px 0 0;
width: 16px;
flex-shrink: 0;
}

.monaco-workbench .part.titlebar>.titlebar-container>.window-title>.command-center .action-item.command-center .left:HOVER,
.monaco-workbench .part.titlebar>.titlebar-container>.window-title>.command-center .action-item.command-center .right:HOVER {
.monaco-workbench .part.titlebar>.titlebar-container>.window-title>.command-center .action-item.command-center:HOVER {
color: var(--vscode-commandCenter-activeForeground);
background-color: var(--vscode-commandCenter-activeBackground);
}
Expand Down