Improve the color schemes page in the SUI #14470

PankajBhojwani · 2022-12-01T02:41:24Z

Summary of the Pull Request

Clicking a color scheme in the list view immediately takes you to the page to edit the scheme
Adding a new scheme immediately takes you to the page to edit the new scheme
'delete' and 'set as default' buttons have been moved to the edit scheme page

PR Checklist

Closes Users _still_ have a hard time with the color schemes page #14289
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Validation Steps Performed

The schemes page still works, keyboard navigation also works

PankajBhojwani · 2022-12-01T02:43:37Z

Screenshots:

PankajBhojwani · 2022-12-01T02:44:05Z

A lot of the 'currently selected scheme' stuff has still been left in because we want to continue to allow keyboard navigation (i.e. we need to know the scheme the user has currently selected with the keyboard without navigating to it, even though the 'currently selected scheme' is quite pointless now if the user is only navigating via mouse).

src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp

PankajBhojwani · 2022-12-01T17:46:34Z

src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml

@@ -36,176 +36,21 @@
        </ResourceDictionary>
    </Page.Resources>

-    <ScrollViewer ViewChanging="ViewChanging">


Putting a ListView inside a ScrollViewer messes things up, so the outer ScrollViewer has been removed in favour of a Grid. Ultimately the only region we want scrolling for anyway is the list of color schemes so this works out

Note to reviewers: hide whitespace to make it easier to review

carlos-zamora

Mainly blocking over the delete button (and disclaimer) placement.

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

carlos-zamora · 2022-12-01T18:13:10Z

src/cascadia/TerminalSettingsEditor/MainPage.cpp

-            contentFrame().Navigate(xaml_typename<Editor::ColorSchemes>(), _colorSchemesPageVM);
            const auto crumb = winrt::make<Breadcrumb>(box_value(clickedItemTag), RS_(L"Nav_ColorSchemes/Content"), BreadcrumbSubPage::None);
            _breadcrumbs.Append(crumb);
+            contentFrame().Navigate(xaml_typename<Editor::ColorSchemes>(), _colorSchemesPageVM);


Why? All the other ones seem to be in the reverse order (navigate, then add crumb).

Turns out there was a race happening that this fixes (the other order results in having the breadcrumb bar be Color Schemes -> Color Schemes when you navigate back to the base page)

carlos-zamora · 2022-12-01T18:18:18Z

src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml

+                <TextBlock x:Name="SelectedSchemeDisclaimer"
+                       Margin="0,4,0,0"
                       VerticalAlignment="Center"
                       Style="{StaticResource DisclaimerStyle}" />


I'm a bit torn. I feel like this disclaimer belongs next to the "delete" button. But I kinda see the value in a disclaimer denoting that the selected scheme is shipped in-box.

Maybe the correct thing to do is to move this disclaimer to be next to the delete button, and (eventually) let them be deleted? My reasoning is that a user shouldn't really care if a scheme is shipped in-box, right? We really just have this right now because we're not ready to handle that yet.

I've made it so the delete button is not even visible for in box schemes! Since now the delete button is in the individual color scheme pages, we can just do that. Also the reason this disclaimer is still here is because of keyboard navigation - we support navigating through the list with arrow keys and hitting the 'delete' button to delete a scheme, so this disclaimer will explain why the 'delete' key doesn't do anything for certain schemes.

carlos-zamora · 2022-12-01T18:19:29Z

src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml

@@ -36,176 +36,21 @@
        </ResourceDictionary>
    </Page.Resources>

-    <ScrollViewer ViewChanging="ViewChanging">


Note to reviewers: hide whitespace to make it easier to review

src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml

src/cascadia/TerminalSettingsEditor/ColorSchemesPageViewModel.idl

carlos-zamora · 2022-12-01T21:45:31Z

src/cascadia/TerminalSettingsEditor/EditColorScheme.xaml

+            </local:SettingContainer>
+
+            <Border MaxWidth="{StaticResource StandardControlMaxWidth}">
+                <Button x:Name="DeleteButton"


Design opinion: instead of using visibility, we should enable/disable the button and display the disclaimer next to it when it's disabled.

The RenameContainer already contains the disclaimer that this is an in box scheme and cannot be renamed/deleted, so I opted to just not show the delete button at all for a cleaner look. Happy to be overridden if the consensus is to show the delete button even if it's disabled

Polling for opinions: @zadjii-msft @DHowett @lhecker

Done, screenshots have been updated

src/cascadia/TerminalSettingsEditor/EditColorScheme.xaml

carlos-zamora · 2022-12-01T21:51:01Z

src/cascadia/TerminalSettingsEditor/EditColorScheme.xaml

+                        Click="{x:Bind ViewModel.SetAsDefault_Click}"/>
+            </local:SettingContainer>
+
+            <Border MaxWidth="{StaticResource StandardControlMaxWidth}">


What do we need the border for?

So that the button gets positioned properly when the width of the window is large. Without the border, the button will be completely left aligned and the list view will be in the center.

src/cascadia/TerminalSettingsEditor/EditColorScheme.cpp

zadjii-msft · 2022-12-01T22:38:39Z

Closes #14289?

PankajBhojwani · 2022-12-01T22:40:38Z

Closes #14289?

Thanks! Updated the description

…/pabhoj/fix_schemes_page

zadjii-msft

I don't have anything to add beyond what carlos already called out.

zadjii-msft · 2022-12-07T16:38:47Z

src/cascadia/TerminalSettingsEditor/ColorSchemes.xaml

-    <ScrollViewer ViewChanging="ViewChanging">
-        <StackPanel Style="{StaticResource SettingsStackStyle}">
-            <TextBlock x:Uid="ColorSchemesDisclaimer"
+    <Grid RowSpacing="8"


note to reviewers: hit that .. The GH diff is egregious compared to the vscode one.

carlos-zamora

Really just blocking over the dumb a11y thing. If my proposal doesn't work, I can try and fix it in a follow-up, just let me know.

Figured I'd ping people for the design-related thread since we're waiting on the a11y fix anyways.

src/cascadia/TerminalSettingsEditor/EditColorScheme.xaml

carlos-zamora · 2022-12-09T21:46:29Z

src/cascadia/TerminalSettingsEditor/EditColorScheme.xaml

+            </local:SettingContainer>
+
+            <Border MaxWidth="{StaticResource StandardControlMaxWidth}">
+                <Button x:Name="DeleteButton"


Polling for opinions: @zadjii-msft @DHowett @lhecker

…/pabhoj/fix_schemes_page

carlos-zamora

Love it! Thanks! Excited to play with this in the bug bash!

src/cascadia/TerminalSettingsEditor/EditColorScheme.xaml

ghost · 2022-12-13T18:48:19Z

Hello @carlos-zamora!

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 · 2023-01-24T18:21:04Z

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

Handy links:

PankajBhojwani added 3 commits November 29, 2022 14:27

list view in grid works now

b754235

disclaimer doesn't shift everything when appearing/disappearing anymore

5bfc81f

simple navigation things

ab8d7dd

PankajBhojwani commented Dec 1, 2022

View reviewed changes

src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp Outdated Show resolved Hide resolved

PankajBhojwani commented Dec 1, 2022

View reviewed changes

carlos-zamora requested changes Dec 1, 2022

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 1, 2022

ghost added Area-Settings UI Anything specific to the SUI Issue-Task It's a feature request, but it doesn't really need a major design. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Dec 1, 2022

PankajBhojwani added 2 commits December 5, 2022 09:41

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

a90c021

…/pabhoj/fix_schemes_page

format

0988f7b

zadjii-msft added the zBugBash-Consider label Dec 6, 2022

zadjii-msft approved these changes Dec 7, 2022

View reviewed changes

PankajBhojwani added 2 commits December 7, 2022 10:58

minor things

0002f85

format

fde1aaf

zadjii-msft assigned carlos-zamora Dec 9, 2022

carlos-zamora requested changes Dec 9, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 9, 2022

carlos-zamora assigned PankajBhojwani and unassigned carlos-zamora Dec 9, 2022

PankajBhojwani added 3 commits December 12, 2022 09:46

Merge branch 'main' of https://github.com/microsoft/terminal into dev…

541f408

…/pabhoj/fix_schemes_page

delete button, remove disclaimer, apply name manually

5ae0c50

format

045b9dc

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 13, 2022

carlos-zamora approved these changes Dec 13, 2022

View reviewed changes

src/cascadia/TerminalSettingsEditor/EditColorScheme.xaml Outdated Show resolved Hide resolved

carlos-zamora unassigned PankajBhojwani Dec 13, 2022

carlos-zamora added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 13, 2022

PankajBhojwani added 2 commits December 13, 2022 10:57

nit

3944085

format

305d253

ghost merged commit decbc0f into main Dec 13, 2022

ghost deleted the dev/pabhoj/fix_schemes_page branch December 13, 2022 21:57

ghost mentioned this pull request Jan 24, 2023

Users _still_ have a hard time with the color schemes page #14289

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the color schemes page in the SUI #14470

Improve the color schemes page in the SUI #14470

PankajBhojwani commented Dec 1, 2022 •

edited

Loading

PankajBhojwani commented Dec 1, 2022 •

edited

Loading

PankajBhojwani commented Dec 1, 2022 •

edited

Loading

PankajBhojwani Dec 1, 2022

carlos-zamora Dec 1, 2022

carlos-zamora left a comment

carlos-zamora Dec 1, 2022

PankajBhojwani Dec 1, 2022

carlos-zamora Dec 1, 2022

PankajBhojwani Dec 1, 2022 •

edited

Loading

carlos-zamora Dec 1, 2022

carlos-zamora Dec 1, 2022

PankajBhojwani Dec 1, 2022 •

edited

Loading

carlos-zamora Dec 9, 2022

PankajBhojwani Dec 13, 2022

carlos-zamora Dec 1, 2022

PankajBhojwani Dec 1, 2022

zadjii-msft commented Dec 1, 2022

PankajBhojwani commented Dec 1, 2022

zadjii-msft left a comment

zadjii-msft Dec 7, 2022

carlos-zamora left a comment

carlos-zamora Dec 9, 2022

carlos-zamora left a comment

ghost commented Dec 13, 2022

ghost commented Jan 24, 2023

Improve the color schemes page in the SUI #14470

Improve the color schemes page in the SUI #14470

Conversation

PankajBhojwani commented Dec 1, 2022 • edited Loading

Summary of the Pull Request

PR Checklist

Validation Steps Performed

PankajBhojwani commented Dec 1, 2022 • edited Loading

PankajBhojwani commented Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PankajBhojwani Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PankajBhojwani Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft commented Dec 1, 2022

PankajBhojwani commented Dec 1, 2022

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

ghost commented Dec 13, 2022

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 commented Jan 24, 2023

PankajBhojwani commented Dec 1, 2022 •

edited

Loading

PankajBhojwani commented Dec 1, 2022 •

edited

Loading

PankajBhojwani commented Dec 1, 2022 •

edited

Loading

PankajBhojwani Dec 1, 2022 •

edited

Loading

PankajBhojwani Dec 1, 2022 •

edited

Loading

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.