Redesign color schemes page by carlos-zamora · Pull Request #9196 · microsoft/terminal

Carlos Zamora (carlos-zamora) · 2021-02-18T02:33:04Z

This PR performs a large overall polish of the color schemes page:

Ensures keyboard navigation is holistically improved (i.e. fully
accessible, no lost focus, etc...)
Adds tooltips and automation properties to all controls
Redesigns the page according to Martin Anderson (@mdtauk)'s approved design
(link).
Note, there are some minor modifications to the design that were
approved by Kayla Cinnamon (@cinnamon-msft).
Automatically reflow's the color buttons when they do not fit in
horizontal mode

Detailed Description of the Pull Request / Additional comments

Redesign
- a data template was introduced to make color representation
  consistent and straightforward
- ContentControl is used to hold a reference to the
  ColorTableEntry and represent it properly using the aforementioned
  data template.
- The design is mainly a StackPanel holding two grids: color table &
  functional colors.
- The color table is populated via code. After much thought, this
  seems to be the easiest way to correctly bind 16 controls that are
  very similar.
- The functional colors are populated via XAML manually.
- We need a grid to separate the text and the buttons. This allows for
  scenarios like "selection background is the longest string" to force
  the buttons and text to be aligned.
Reflow
- A VisualStateManager uses an AdaptiveTrigger to change the
  orientation of the color tables' stack panel. The adaptive trigger
  was carefully calculated to align with the navigation view's
  breakpoint.
Keyboard Navigation
- (a lesson from SettingContainer) ContentControl can be focused
  as opposed to the content inside of it. We don't want that, so we
  set IsTabStop to false on it. That basically fixes all of our
  keyboard navigation issues in this new design.
Automation Properties and ToolTips
- As in my previous PRs, I can't seem to figure out how to bind to a
  control's automation property to its own tooltip. So I had to do
  this in the code and add a few x:Name around.

Validation Steps Performed

Manually tested...
- tab navigation
- accessibility insights
- NVDA
- changing color schemes updates color table
specific scenario:
- change a color table color and a functional color
- navigate to a different color scheme
- navigate back to the first color scheme
- if the colors persist, the changes were propagated to the settings model

References #8997 - Based on the work from Eric Tian (@Chips1234)
References #6800 - Settings UI Epic
Closes #8765 - Polish Color Schemes page
Closes #8899 - Automation Properties
Closes #8768 - Keyboard Navigation

Carlos Zamora (carlos-zamora) · 2021-02-18T02:41:26Z

Demos

Dustin L. Howett (DHowett) · 2021-02-18T06:19:19Z

Note, there are some minor modifications to the design that were approved by Kayla Cinnamon (@cinnamon-msft).

just for awareness, can you explain what they were and where they were approved?

Heiko (htcfreek) · 2021-02-18T12:15:37Z

Carlos Zamora (@carlos-zamora)
I am a bit confused on this PR because #8997 isn't ready to merge.

There are some ideas not implemented (column title, improved tooltip).
You have copied a bug into your PR that isn't fixed completely in [Settings UI] [Colour schemes page] Redesign Colour Buttons #8997.
(See my reviews on the other PR.)

Kayla Cinnamon (cinnamon-msft) · 2021-02-18T16:43:20Z

Tweaks to Martin Anderson (@mdtauk)'s design include moving the color buttons closer to the labels on the left and also making the "Color table" and "Functional colors" text subtitles. Also, we moved the functional colors closer to the color table to keep it more compact.

Carlos Zamora (carlos-zamora) · 2021-02-18T17:08:48Z

Carlos Zamora (@carlos-zamora)
I am a bit confused on this PR because #8997 isn't ready to merge.

There are some ideas not implemented (column title, improved tooltip).

You have copied a bug into your PR that isn't fixed completely in [Settings UI] [Colour schemes page] Redesign Colour Buttons #8997.
(See my reviews on the other PR.)

Sorry for the confusion. I gave #8997 a deadline of 2/15 here because it's blocking closing a few bugs for the Settings UI. As we're preparing for release, I'd like to make sure the keyboard navigation and accessibility story are complete, and the redesign kinda encompasses that.

Regarding some other feedback from #8997...

From this comment:

I think you should add a small header above the two columns to indicate that the first color column are the non-bright version and the second color column are the bright version of the color.

Personally, I think adding the extra small headers would make it feel more cluttered. The format of the color table is already standard across terminal color schemes. That said, to make it more inclusive, maybe a link to the docs or a small description would be nice? Kayla Cinnamon (@cinnamon-msft) Thoughts?

From this comment:

I have looked at the tooltip text and want to bring up one point: As example the tooltip for the red non-bright color element says "red". But isn't this confusing if I choose an other color because I create a special color sheme? (Example: color=purple, tooltip say "red")

This could be a special problem for people with eye problems like people with a red-green weakness.

First off, I'm trying to minimize localization changes so if we go with this, I'll want it to be a follow up. Aside from that, this really is the same kind of issue as your last comment. "Red" actually being some hex code that doesn't look red at all is pretty standard across terminals. In fact, when you use something like PowerShell and get the "Red" color, you just get the color table's "red", not the actual red color. Again, in an effort to make it more inclusive, I'd say adding a link to the docs or a small description might work. Kayla Cinnamon (@cinnamon-msft) Thoughts?

Alternatively, we could extract the hex value from the color picker and display that as the tooltip. But I don't see much value to it, and I think this would be better as a follow-up if we decide to go with it.

Kayla Cinnamon (cinnamon-msft) · 2021-02-18T20:06:15Z

From this comment:

I think you should add a small header above the two columns to indicate that the first color column are the non-bright version and the second color column are the bright version of the color.

Personally, I think adding the extra small headers would make it feel more cluttered. The format of the color table is already standard across terminal color schemes. That said, to make it more inclusive, maybe a link to the docs or a small description would be nice? Kayla Cinnamon (@cinnamon-msft) Thoughts?

I know we want to eventually add docs links to the right side of all of the pages, so maybe we hold off until then? I agree that adding headers over each color may be too cluttered.

From this comment:

I have looked at the tooltip text and want to bring up one point: As example the tooltip for the red non-bright color element says "red". But isn't this confusing if I choose an other color because I create a special color sheme? (Example: color=purple, tooltip say "red")
This could be a special problem for people with eye problems like people with a red-green weakness.

First off, I'm trying to minimize localization changes so if we go with this, I'll want it to be a follow up. Aside from that, this really is the same kind of issue as your last comment. "Red" actually being some hex code that doesn't look red at all is pretty standard across terminals. In fact, when you use something like PowerShell and get the "Red" color, you just get the color table's "red", not the actual red color. Again, in an effort to make it more inclusive, I'd say adding a link to the docs or a small description might work. Kayla Cinnamon (@cinnamon-msft) Thoughts?

Same as my comment above. The colors in the tooltips align directly with how shells interpret their colors, for example Ubuntu uses "blue" and "green" as its colors in the prompt. This means it'll display whatever is set to "blue" and "green" even though they may not actually be blue and green, which is totally fine.

Dustin L. Howett (DHowett) · 2021-02-18T20:19:13Z

Same as my comment above. The colors in the tooltips align directly with how shells interpret their colors, for example Ubuntu uses "blue" and "green" as its colors in the prompt. This means it'll display whatever is set to "blue" and "green" even though they may not actually be blue and green, which is totally fine.

Sorta yes, sorta no. Ubuntu uses "four" and "two" as the colors for its prompt. The more correct expression for this is, like... "ANSI Color 0 (Usually Black)" "ANSI Color 7 (Usually White)"

Dustin L. Howett (DHowett) · 2021-02-18T20:19:33Z

But we accept as a shortcut the "usually" part, and boil it down to Red. Bright red. Black. "Bright black", whatever that is.

Dustin L. Howett (DHowett)

VERY, very clever with the data templates. That's excellent work. Big/small nits.

Dustin L. Howett (DHowett) · 2021-02-18T20:25:44Z

+        Automation::AutomationProperties::SetName(DeleteButton(), RS_(L"ColorScheme_DeleteButton/Text"));
+    }
+
+    void ColorSchemes::SizeChanged(IInspectable const& /*sender*/, Windows::UI::Xaml::SizeChangedEventArgs const& e)


sorry, we couldn't use a VSM because what now? we would have to retemplate the entire thing? heck naw

The VSM honestly just made the code more complex to change this one value. The changes would look something like this...

add the VSM; define 2 states (compact and standard)

Either...

set up an AdaptiveTrigger to switch between the states (I'd probably have to bind that to the _colorPanelHorizontalWidth + indentMargin, which would be annoying to set up in the code)

set up SizeChanged (like we do here), and call VisualStateManager::GoToState()

Reference: https://docs.microsoft.com/en-us/windows/uwp/design/layout/layouts-with-xaml#adaptive-layouts-with-visual-states-and-state-triggers

Carlos Zamora (carlos-zamora) · 2021-02-18T21:58:14Z

Mike Griese (zadjii-msft)

Okay I have a lot of questions and it's 5:09 here

Heiko (htcfreek) · 2021-02-19T10:21:55Z

Same as my comment above. The colors in the tooltips align directly with how shells interpret their colors, for example Ubuntu uses "blue" and "green" as its colors in the prompt. This means it'll display whatever is set to "blue" and "green" even though they may not actually be blue and green, which is totally fine.

Sorta yes, sorta no. Ubuntu uses "four" and "two" as the colors for its prompt. The more correct expression for this is, like... "ANSI Color 0 (Usually Black)" "ANSI Color 7 (Usually White)"

We could use "Show color type as ..." so for light red the tooltipp says "Show light red as ...". This makes clear what the tooltip means.

But I am okay when you let it as is. If the users are confused they will fill issues and the you can think on a solution again.

Mike Griese (zadjii-msft)

Now this, I like this

Deleted user (ghost) · 2021-02-19T18:20:01Z

Hello Dustin L. Howett (@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.

Deleted user (ghost) · 2021-03-01T18:02:24Z

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

Handy links:

Carlos Zamora and others added 5 commits February 17, 2021 15:45

Redesign color schemes page

164d84e

redesign color table

7b3f6d1

add automatic reflow

e8ac4a7

add tooltips and automation properties

1071c46

add remaining tooltips and automation properties

ca6ec7a

This comment has been minimized.

Sign in to view

Carlos Zamora (carlos-zamora) commented Feb 18, 2021

View reviewed changes

Comment thread src/cascadia/TerminalSettingsEditor/ColorSchemes.cpp Resolved

Dustin L. Howett (DHowett) requested changes Feb 18, 2021

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 Feb 18, 2021

address Dustin's feedback

cf2ad59

Carlos Zamora (carlos-zamora) assigned Dustin L. Howett (DHowett) Feb 18, 2021

Mike Griese (zadjii-msft) reviewed Feb 18, 2021

View reviewed changes

Carlos Zamora added 2 commits February 18, 2021 15:34

match adaptive trigger to navigation view trigger

319b990

address Griese feedback

293b336

fix system color binding

9db6633

Heiko (htcfreek) mentioned this pull request Feb 19, 2021

[Settings UI] [Colour schemes page] Redesign Colour Buttons #8997

Closed

2 of 3 tasks

Mike Griese (zadjii-msft) approved these changes Feb 19, 2021

View reviewed changes

Dustin L. Howett (DHowett) approved these changes Feb 19, 2021

View reviewed changes

Dustin L. Howett (DHowett) added the AutoMerge Marked for automatic merge by the bot when requirements are met label Feb 19, 2021

ghost merged commit bb0e1d3 into main Feb 19, 2021

ghost deleted the dev/cazamor/sui/color-schemes-redesign branch February 19, 2021 18:20

This was referenced Mar 1, 2021

Polish Settings UI: Keyboard navigation (w/ tab key) #8768

Closed

Add automation properties to Settings UI #8899

Closed

Polish Settings UI: Color Scheme page #8765

Closed

This pull request was closed.

Conversation

Carlos Zamora (carlos-zamora) commented Feb 18, 2021 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

Carlos Zamora (carlos-zamora) commented Feb 18, 2021

Demos

Uh oh!

Dustin L. Howett (DHowett) commented Feb 18, 2021

Uh oh!

Heiko (htcfreek) commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kayla Cinnamon (cinnamon-msft) commented Feb 18, 2021

Uh oh!

Carlos Zamora (carlos-zamora) commented Feb 18, 2021

Uh oh!

This comment has been minimized.

Uh oh!

Kayla Cinnamon (cinnamon-msft) commented Feb 18, 2021

Uh oh!

Dustin L. Howett (DHowett) commented Feb 18, 2021

Uh oh!

Dustin L. Howett (DHowett) commented Feb 18, 2021

Uh oh!

Dustin L. Howett (DHowett) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Dustin L. Howett (DHowett) Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

Carlos Zamora (carlos-zamora) Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Carlos Zamora (carlos-zamora) commented Feb 18, 2021

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Heiko (htcfreek) commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mike Griese (zadjii-msft) left a comment

Choose a reason for hiding this comment

Uh oh!

Deleted user (ghost) commented Feb 19, 2021

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.

Uh oh!

Deleted user (ghost) commented Mar 1, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Carlos Zamora (carlos-zamora) commented Feb 18, 2021 •

edited by DHowett

Loading

Heiko (htcfreek) commented Feb 18, 2021 •

edited

Loading

Heiko (htcfreek) commented Feb 19, 2021 •

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.