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 nullable colors and improve Profile.Icon in settings UI #17870

Merged
merged 21 commits into from
Dec 12, 2024

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Sep 6, 2024

Summary of the Pull Request

Adds some pre-existing settings ($profile.foreground, $profile.background, $profile.selectionBackground, $profile.cursorColor) to the settings UI. This was accomplished by introducing a new control: NullableColorPicker. This control allows the user to pick a color from the color scheme, set the color to null, and select a color from an advanced color picker.

Improves the UI for the Profile.Icon setting by adding an "Icon Type" combo box. This allows the user to pick from multiple options:

  • None: sets the icon to "none" which is interpreted as no icon
  • Built-in Icon: presents a combo box that enumerates the Segoe MDL 2 assets
  • Emoji: presents a text box with a hint to open the emoji picker
  • File: presents a text box to input the path of the image to use

Additionally, the rendered icon is displayed in the setting container. If "none", "none" is presented to the user (localized).

References and Relevant Issues

#10000

Detailed Description of the Pull Request / Additional comments

  • NullableColorPicker control
    • includes a built-in NullColorButton to set the current value to null
    • includes a "More colors..." button to display an advanced color picker
    • uses data templates on data templates (data templates squared?) to convert the current color scheme into a grid of color chips
    • color chips display a checkmark (similar to Windows settings personalization). This automatically updates its color to stay compliant with color contrast.
    • color chips are added to a list so we can (un)check them when a new color is selected
  • SettingsContainer changes
    • Forked ExpanderSettingContainerStyle to allow for a custom preview template. This way, we can display the current value in the expander and we're not just limited to text.
    • changed type of CurrentValue property from String to IInspectable
    • added CurrentValueTemplate property to control how to display the current value
  • Miscellaneous:
    • Added a few converters (BooleanToVisibility, ColorToString, ColorToBrush)
    • Added NameWithHexCode to ColorTableEntry to expose a color as Red #RRGGBB (used for tooltips and a11y)
    • Added ForegroundPreview (and equivalent for other colors) to AppearanceViewModel to deduce the color that will be used

Validation Steps Performed

  • a11y pass (NVDA, keyboard)
  • set the color to one of the color chips
  • set the color to null
  • set the color to a value from the integrated color picker
  • control updates properly when a new color scheme is selected
  • control updates properly when a color scheme has multiple colors of the same value

Follow-ups

  • [A11y] Screen readers don't read expander's preview text
  • Add Tab Color to settings UI
  • Update CursorColor preview to display #FFFFFF as "invert"
  • Use Leonard's font picker UI, with the Segoe icon picker, so that you can filter the list

@carlos-zamora

This comment was marked as outdated.

@carlos-zamora carlos-zamora mentioned this pull request Sep 18, 2024
65 tasks
Windows::Foundation::IInspectable ColorToBrushConverter::Convert(Windows::Foundation::IInspectable const& value, Windows::UI::Xaml::Interop::TypeName const& /*targetType*/, Windows::Foundation::IInspectable const& /*parameter*/, hstring const& /*language*/)
{
const auto color = value.as<Windows::UI::Color>();
return Microsoft::Terminal::UI::Converters::ColorToBrush(color);
Copy link
Member

Choose a reason for hiding this comment

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

fwiw if we're adding a static global converter function, we don't need a converter object!

Copy link
Member Author

Choose a reason for hiding this comment

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

Omg, yeah, I hate it. I'd rather directly use mtu:Converters.ColorToBrush(<param>) in the XML, but the problem is here:

<!--  CommonResources.xaml::ColorPreviewTemplate  -->
Fill="{Binding Converter={StaticResource ColorToBrushConverter}}"

We don't have a Path set because we want to convert the object itself. Since there isn't a path, there isn't a way to target the object itself (from what I know, at least). Fill="{Binding mtu:Converters.ColorToBrush}" silently fails (no rendered Rectangle)!

Also, if you're curious, we can't use x:Bind because XamlCompiler error WMC1119: This Xaml file must have a code-behind class to use {x:Bind}. See http://go.microsoft.com/fwlink/?LinkID=532920&clcid=0x409 for more information.

Also also, in case you're extra curious, I wasn't able to figure out how to define the DataType as IReference<Core::Color> because of the IReference part.

So that's why this part's implemented like this 🫤

Copy link
Member

Choose a reason for hiding this comment

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

UGH. Thanks.

@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 Oct 3, 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 Oct 4, 2024
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.

18/26

src/cascadia/TerminalSettingsEditor/Appearances.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/Appearances.xaml Outdated Show resolved Hide resolved

<ContentPresenter Grid.Column="0"
Content="{x:Bind ColorSchemeVM, Mode=OneWay}"
ContentTemplate="{StaticResource ColorSchemeTemplate}" />
Copy link
Member

Choose a reason for hiding this comment

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

📝 ColorSchemeTemplate?

@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 Oct 11, 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 Oct 14, 2024
## Summary of the Pull Request
Improves the UI for the Profile.Icon setting by adding an "Icon Type"
combo box. This allows the user to pick from multiple options:
- None: sets the icon to "none" which is interpreted as no icon
- Built-in Icon: presents a combo box that enumerates the Segoe MDL 2
assets
- Emoji: presents a text box with a hint to open the emoji picker
- File: presents a text box to input the path of the image to use

Additionally, the rendered icon is displayed in the setting container.
If "none", "none" is presented to the user (localized).
✅ Verified as accessible using Accessibility Insights
#10000
@carlos-zamora
Copy link
Member Author

Alright! Made some minor changes to fix the focus issue, text box issue, theme issue (on Win10), and layout concerns. Here's a storyboard of the slightly new design:

{B9992E97-E737-413C-ACA6-0870655DEC4B}

{9835A2BA-83AA-419F-BD62-34A75C533001}

{1819AC75-118B-406A-8600-465C3B5660A8}

{5738429C-7998-415A-8E82-1B69705C0B03}

Changes:

  • Moved color scheme back to the top
  • Removed text boxes from color picker. IMO this is fine as the hex value is presented in the Expander's header.
    • I do still miss having the text box though. What if I add a textbox inside the expander? That'll act as a live preview for the hex value too, and allow users to copy-paste the value directly? It may look a little weird though :/
  • Combined override settings and color scheme setting into one expander
    • Weird idea? Maybe. But there are plenty of examples of expanders having full controls in the header. This groups the settings together, but keeps them all near the top (by the live preview too).

@carlos-zamora
Copy link
Member Author

carlos-zamora commented Nov 19, 2024

Feedback from Bug Bash (11/19)

  • Nit: "Color" --> "Cursor Color"
  • Foreground/Background/Selection background don't get updated when the selected scheme changes
  • The Icon editor doesn't update when I click the revert button (!)
  • Icon/ColorScheme SettingContainer has wrong indentation in header (No repro, probably a bad merge)
  • Nit, probably a puntable thought: can we do Leonard's font picker thing, with the Segoe icon picker, so that you can filter the list? (i may be overindexed on filterable list UIs these days) (Moved as a follow-up)

@carlos-zamora carlos-zamora changed the title Add nullable colors to settings UI Add nullable colors and improve Profile.Icon in settings UI Nov 21, 2024
@carlos-zamora
Copy link
Member Author

Note to reviewer

The Profile.Icon changes all come from #17965 which merged into this PR. The PR title and description have been appropriately updated.

Some changes have been made to that area since it merged (just this one commit really): e46ed40

@DHowett
Copy link
Member

DHowett commented Dec 11, 2024

(Code format seems upset)

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.

Excellent work!

@DHowett DHowett assigned carlos-zamora and unassigned DHowett Dec 12, 2024
@carlos-zamora carlos-zamora merged commit 5132f9c into main Dec 12, 2024
19 of 20 checks passed
@carlos-zamora carlos-zamora deleted the dev/cazamor/SUI/nullable-color-picker branch December 12, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants