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

Already on GitHub? Sign in to your account

Jump to bottom

Add color chips to the color scheme dropdown in Appearance #14587

Merged
11 commits merged into from
Jan 19, 2023
Merged

Add color chips to the color scheme dropdown in Appearance #14587

11 commits merged into from
Jan 19, 2023

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Dec 20, 2022 •
edited by DHowett
Loading

Does what it says on the tin.

@PankajBhojwani
Copy link
Contributor Author

chipDropdown

@carlos-zamora
Copy link
Member

Since the XAML is very similar to that of the color schemes page, is there any way we can deduplicate the code? Maybe introduce something similar to SettingContainer (i.e. ColorSchemeComboBoxItem) where it takes in a ColorSchemeViewModel and presents it in such a way?

@zadjii-msft
Copy link
Member

oh it's so pretty

@PankajBhojwani
Copy link
Contributor Author

Since the XAML is very similar to that of the color schemes page, is there any way we can deduplicate the code? Maybe introduce something similar to SettingContainer (i.e. ColorSchemeComboBoxItem) where it takes in a ColorSchemeViewModel and presents it in such a way?

Since the values are slightly different between the two UIs (namely the sizes/spacings), we will need 2 separate styles, one for the color schemes list view and one for the color schemes combo box.... This means we are simply moving the code duplication from one place to another.

carlos-zamora
carlos-zamora approved these changes Jan 10, 2023
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Eh, the deduplication of code seems like a decent amount of extra work for little gain. I don't even know if it's worth filing a follow-up. If anybody else agrees, sure. But I don't think it's worth blocking over.

Excited to see this merge!

src/cascadia/TerminalSettingsEditor/Appearances.xaml Show resolved Hide resolved
@zadjii-msft zadjii-msft added this to the Terminal v1.17 milestone Jan 10, 2023
@DHowett
Copy link
Member

DHowett commented Jan 13, 2023

Wow, that other PR made this one super easy

carlos-zamora added a commit that referenced this pull request Jan 14, 2023
@carlos-zamora
PRE-MERGE #14587 Add color chips to the color scheme dropdown in Prof…
7aa3504 
…ile -> Appearance
@carlos-zamora carlos-zamora mentioned this pull request Jan 17, 2023
Closed
@carlos-zamora
Copy link
Member

Bug Bash Feedback:

  • DH: I feel like the padding between the list cell and the background could be smaller and more even on all sides

Base automatically changed from dev/pabhoj/use_csvm_in_profiles to main January 18, 2023 21:30
@DHowett DHowett changed the title Add color chips to the color scheme dropdown in Profile -> Appearance Add color chips to the color scheme dropdown in Appearance Jan 18, 2023
@PankajBhojwani
Copy link
Contributor Author

DH: I feel like the padding between the list cell and the background could be smaller and more even on all sides

We can change the Padding property of the ComboBox and make the spacing smaller when the combo box is unexpanded, like so:

image

However, for some reason this does not translate over to when the ComboBox is expanded, I have a feeling xaml is overriding us for some reason

image

DHowett
DHowett reviewed Jan 19, 2023
View reviewed changes
src/cascadia/TerminalSettingsEditor/Appearances.xaml
@@ -54,6 +54,7 @@
SettingOverrideSource="{x:Bind Appearance.DarkColorSchemeNameOverrideSource, Mode=OneWay}">
<ComboBox ItemsSource="{x:Bind Appearance.SchemesList, Mode=OneWay}"
SelectedItem="{x:Bind Appearance.CurrentColorScheme, Mode=TwoWay}"
Padding="4"
Style="{StaticResource ComboBoxSettingStyle}">
<ComboBox.ItemTemplate>
<DataTemplate x:DataType="local:ColorSchemeViewModel">
Copy link
Member

Choose a reason for hiding this comment

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

i was thinking more like, we could reduce the inner padding inside the grid to generally tighten it up :)

Copy link
Member

Choose a reason for hiding this comment

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

nah, it looks fine. let's ship it.

@DHowett
Copy link
Member

DHowett commented Jan 19, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 19, 2023
@ghost
Copy link

ghost commented Jan 19, 2023

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 16fe2e5 into main Jan 19, 2023
@ghost ghost deleted the dev/pabhoj/color_chips_dropdown branch January 19, 2023 21:47
@ghost
Copy link

ghost commented Jan 24, 2023

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees

@DHowett DHowett

Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Milestone
Terminal v1.17
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@PankajBhojwani @carlos-zamora @zadjii-msft @DHowett