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.

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

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

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

TylerLeonhardt
Copy link
Member

Fixes #163957

@TylerLeonhardt TylerLeonhardt enabled auto-merge (squash) October 24, 2022 11:27
@TylerLeonhardt TylerLeonhardt self-assigned this Oct 24, 2022
}
// 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

@daviddossett daviddossett added this to the November 2022 milestone Oct 26, 2022
@TylerLeonhardt TylerLeonhardt merged commit ad114d7 into main Nov 1, 2022
@TylerLeonhardt TylerLeonhardt deleted the tyler/supreme-chimpanzee branch November 1, 2022 08:40
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove ? dropdown from command center
3 participants