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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tab color indicator for tab switch menu (CTRL+Tab) #17820

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

nukoseer
Copy link
Contributor

Added tab color indicator for the tab switch menu. Tab color indicators have the same color as the background color of the tabs. If a tab has the default background color, the indicator is not shown in the tab switch menu.

I don't know if it was a good idea, but I just used small circles for indicators.

Relevant issue(s); #17465

20240828_231729.mp4

Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

Neat. 😺

@nukoseer nukoseer changed the title Add tab color indicator for tab switch menu (CTRL+T) Add tab color indicator for tab switch menu (CTRL+Tab) Aug 29, 2024
@nukoseer nukoseer force-pushed the dev/ctrl_tab_color_indicator branch from c5e01b0 to d25e78b Compare August 29, 2024 13:05
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Yea, these are just nits. This is neat, thanks!

@@ -205,6 +205,13 @@
Visibility="{x:Bind Item.(local:TabPaletteItem.TabStatus).IsInputBroadcastActive, Mode=OneWay}" />

</StackPanel>

<Ellipse Grid.Column="4"
Copy link
Member

Choose a reason for hiding this comment

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

Slightly worried that this will overlap with the scrollbar on the tab picker if there's a lot of tabs. Who knows - maybe that column was vestigial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It kind of overlaps, but it doesn't seem like a big problem to me 😅

resim

{
const auto currentColor = GetTabColor();

if (currentColor.has_value())
Copy link
Member

Choose a reason for hiding this comment

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

IIRC this if/else could probably just be a single .value_or(Windows::UI::Colors::Transparent())

@DHowett
Copy link
Member

DHowett commented Sep 4, 2024

I love this so much! Thanks for doing it! 😄

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Hey, I did just realize that we have TabPaletteItem specifically for things like this. You can see where it's used by finding its members IsReadOnlyActive and IsInputBroadcastActive.

I was feeling a little weird about it being on the generic PaletteItem, because that's also used for commands. I'm thinking that it might feel better on TabPaletteItem!

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 4, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Sep 5, 2024
@nukoseer
Copy link
Contributor Author

nukoseer commented Sep 5, 2024

Hey, I did just realize that we have TabPaletteItem specifically for things like this. You can see where it's used by finding its members IsReadOnlyActive and IsInputBroadcastActive.

I removed TabColorIndicator from PaletteItem and moved it to TerminalTabStatus. I also moved the code from TabBase.cpp to TerminalTab.cpp. Sorry for the confusion. I hope it looks better now.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Beautiful! Only +13 lines, wow

@DHowett DHowett merged commit 544452d into microsoft:main Sep 6, 2024
15 checks passed
DHowett pushed a commit that referenced this pull request Sep 6, 2024
Added tab color indicator for the tab switch menu. Tab color indicators
have the same color as the background color of the tabs. If a tab has
the default background color, the indicator is not shown in the tab
switch menu.

Closes #17465

(cherry picked from commit 544452d)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgS0CZA
Service-Version: 1.22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants