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

Polish Settings UI: Color Scheme page #8765

Closed
3 of 7 tasks
carlos-zamora opened this issue Jan 12, 2021 · 2 comments · Fixed by #9196
Closed
3 of 7 tasks

Polish Settings UI: Color Scheme page #8765

carlos-zamora opened this issue Jan 12, 2021 · 2 comments · Fixed by #9196
Labels
Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone
Terminal v1.7

Comments

@carlos-zamora
Copy link
Member

carlos-zamora commented Jan 12, 2021 •
edited
Loading

  • Edit a color scheme -> Hit 'apply' -> the selected color scheme resets to the first color scheme in the list (instead of the one just edited)
  • The buttons turn gray on rollover covering up what color I'm looking at (I have dark mode)
  • Activate scheme renamer… then commit with enter key or dismiss with esc key and it will do the needful then go highlight the Discard Changes button at the bottom. That's a bit weird.
  • Rename/add new buttons are misaligned with combobox
    • misaligned buttons
  • Reorganize Color Scheme Entries
    • There should be 3 tables: non-bright colors, bright colors, and miscellaneous.
    • Currently, everything is bound to one color table. It would be nice to find a way to fix that.
  • Redesign color buttons
    • remove text for each color button (to prevent cropping when text is localized)
    • instead, add a tooltip (and automation property) that includes the label
  • "add new" button may be cropped when localized (needs verification after Update tooltips and setting names in SUI #8777)
@carlos-zamora carlos-zamora added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-3 A description (P3) Area-SettingsUI Anything specific to the SUI labels Jan 12, 2021
@carlos-zamora carlos-zamora added this to the Terminal v1.6 milestone Jan 12, 2021
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 12, 2021
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jan 12, 2021
@zadjii-msft zadjii-msft self-assigned this Jan 13, 2021
@zadjii-msft
Copy link
Member

What have we learned? (Jan 13)

  • When you navigate to a new item in the NavigationView, we ctor a new version of that Page. That explains why adding the schemes to the list each time just worked - each navigation is starting fresh.
  • Same deal happens when we save the settings. We end up re-navigating, which causes the page to get reconstructed, which makes it impossible to store state in the page across navigations.

Let's try sticking that state in ColorSchemesPageNavigationState, and not make a new one every time we Navigate. If the MainPage just hangs onto a single copy of ColorSchemesPageNavigationState, then we can stick some data in there to be able to reconstruct the state. I'm assuming that's what it's for.

@zadjii-msft zadjii-msft mentioned this issue Jan 15, 2021
Merged
4 tasks
ghost pushed a commit that referenced this issue Jan 19, 2021
@zadjii-msft
Persist selected color scheme on navigation; Don't gray-out color swa…
9293867 
…tches (#8799)

## Summary of the Pull Request

This PR fixes two of the components of #8765. 

> * [ ] Edit a color scheme -> Hit 'apply' -> the selected color scheme resets to the first color scheme in the list (instead of the one just edited)

This was fixed by storing the navigation state as a singleton in MainPage, and having the color schemes page update the selected scheme on that singleton. That way, a subsequent navigation to the schemes page could re-use the existing state.

> * [ ] The buttons turn gray on rollover covering up what color I'm looking at (I have dark mode)

This one was tricky. We're binding the resource for this button, to the color the button is bound to. We're also running a converter on that color, as to change the alpha slightly. This allows us to still have visual feedback on pointerover, without obscuring the color entirely. 

## PR Checklist
* [x] I work here
* [x] Tested manually
@zadjii-msft zadjii-msft removed their assignment Jan 19, 2021
This was referenced Jan 21, 2021
Closed
Open
mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
@zadjii-msft @mpela81
Persist selected color scheme on navigation; Don't gray-out color swa…
1f8e660 
…tches (microsoft#8799)

## Summary of the Pull Request

This PR fixes two of the components of microsoft#8765. 

> * [ ] Edit a color scheme -> Hit 'apply' -> the selected color scheme resets to the first color scheme in the list (instead of the one just edited)

This was fixed by storing the navigation state as a singleton in MainPage, and having the color schemes page update the selected scheme on that singleton. That way, a subsequent navigation to the schemes page could re-use the existing state.

> * [ ] The buttons turn gray on rollover covering up what color I'm looking at (I have dark mode)

This one was tricky. We're binding the resource for this button, to the color the button is bound to. We're also running a converter on that color, as to change the alpha slightly. This allows us to still have visual feedback on pointerover, without obscuring the color entirely. 

## PR Checklist
* [x] I work here
* [x] Tested manually
@Chips1234 Chips1234 mentioned this issue Feb 2, 2021
Closed
3 tasks
@carlos-zamora carlos-zamora mentioned this issue Feb 11, 2021
Merged
ghost pushed a commit that referenced this issue Feb 17, 2021
@carlos-zamora
Perform polish of miscellaneous Settings UI bugs (#9126)
557edc6 
- 0b0dbdf Makes the browse buttons center vertically aligned
  - This is now made possible by #8919. The "center" used to include the height of the header. Now that it's separated, the center is solely calculated to be the text box.
  - Closes #8764 
- 0288f06 Fix keyboard navigation focus for color schemes rename button
  - Enter/Esc when in the scheme renamer now focuses the combo box
  - Keyboard-invoking accept/cancel button focuses the rename button
  - References #8765 and #8768
- d5ef552 Cyclical tab navigation
  - now, if you try to tab past the save button, you cycle back to the beginning of the navigation view
  - this is consistent with the xaml controls gallery
  - References #8768 
- a613b08 AutomationProperties for Save, Reset, and open json buttons
  - References #8899
@carlos-zamora carlos-zamora mentioned this issue Feb 18, 2021
Merged
@ghost ghost added the In-PR This issue has a related PR label Feb 18, 2021
@ghost ghost closed this as completed in #9196 Feb 19, 2021
@ghost ghost removed the In-PR This issue has a related PR label Feb 19, 2021
ghost pushed a commit that referenced this issue Feb 19, 2021
@carlos-zamora
Redesign color schemes page (#9196)
bb0e1d3 
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 @mdtauk's approved design
  ([link](#8997 (comment))).
  Note, there are some minor modifications to the design that were
  approved by @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 @Chips1234
References #6800 - Settings UI Epic
Closes #8765 - Polish Color Schemes page
Closes #8899 - Automation Properties
Closes #8768 - Keyboard Navigation
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Feb 19, 2021
@ghost
Copy link

ghost commented Mar 1, 2021

🎉This issue was addressed in #9196, which has now been successfully released as Windows Terminal Preview v1.7.572.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-SettingsUI Anything specific to the SUI Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
Terminal v1.7
Development

Successfully merging a pull request may close this issue.

Redesign color schemes page microsoft/terminal
3 participants
@carlos-zamora @zadjii-msft @cinnamon-msft