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

Rejuvenate the color schemes page #13269

Merged
merged 76 commits into from
Aug 31, 2022
Merged

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Jun 10, 2022

Update the color schemes page with the new Win 11 style.
With the rejuvenated experience, we now have two pages:

  • ColorSchemes.xaml: "color scheme selection" page
    • This is the starting page for the "color schemes" nav view
      item. It is intended to have the user select a color scheme
      they want to modify. The user can also click the "Add new"
      button to add a new color scheme or the "delete" button to
      delete the selected scheme.
    • If a scheme cannot be deleted, the delete button is disabled
      and a disclaimer is shown.
    • A "set as default" button sets the selected scheme as the
      color scheme for profiles.default (aka "base layer").
    • The list view item for each scheme includes the name of the
      scheme, the default tag (if the scheme is the one set in base
      layer), a text preview of the foreground/background, and a
      grid of 16 color chips showing the colors for the scheme.
    • Implementation details:
      • View - ColorSchemes:
        • "Enter" --> edit the selected scheme
        • "Delete" --> delete the selected scheme
        • if the selected scheme cannot be deleted, we
          show the disclaimer
      • View model - ColorSchemesPageViewModel
        • when possible, the XAML binds directly to the
          view model functions. Thus, we include logic
          to delete, edit, and set the selected scheme
          as default.
        • store the current page, so that we know which
          page to navigate to upon saving/discarding
          changes.
  • EditColorScheme.xaml: "color scheme modification" page
    • a terminal preview of the color scheme is shown at the top of
      the page.
    • all colors for the scheme are displayed as color chips in an
      expander that starts as expanded.
    • renaming a color scheme is also inside an expander, but
      there's no need for "renaming mode" anymore
    • Implementation details:
      • View - EditColorScheme:
        • include logic to display the disclaimer and
          add the automation properties
        • include logic for "Enter" and "Escape" in the
          rename editor
      • View Model - ColorSchemeViewModel:
        • as before, when possible, the XAML binds
          directly to the view model functions.
        • To enable the "default" tag functionality, we
          had to expose knowledge of being the default
          via "IsDefaultScheme()" which compares the
          current name to the one in the settings model.
  • Miscellaneous implementation details:
    • to get the expander to start as expanded, we had to modify the
      setting container style.
    • Since "set as default" is a button on the selector page, we
      needed a way to refresh the view model's knowledge of being
      the default. So we added a RefreshIsDefault API to the
      ColorSchemeViewModel to notify changes.
    • With the new layout, we no longer need an 'enter rename mode'
      button, so all that logic has been removed
    • Add logic to MainPage to handle navigating to the correct
      page upon saving/discarding changes.

Closes #9775

Co-authored-by: Carlos Zamora cazamor@microsoft.com

@PankajBhojwani PankajBhojwani changed the title start revamping cs page Rejuvenate the color schemes page in the SUI to follow Win 11 style guidelines Jun 10, 2022
@carlos-zamora
Copy link
Member

Screenshots for 5a6ab36
image
image

@@ -49,6 +50,16 @@ namespace winrt::Microsoft::Terminal::Settings::Editor::implementation
return _Name;
}

bool ColorSchemeViewModel::IsDefaultScheme()
{
return _Name == _settings.ProfileDefaults().DefaultAppearance().ColorSchemeName();
Copy link
Member

Choose a reason for hiding this comment

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

clever. renaming a scheme updates all profiles using it, so this will be true after a rename too. very clever.

@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Aug 31, 2022
@ghost ghost requested a review from lhecker August 31, 2022 18:40
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.

Let's do this and fix the issues in post eh?

@DHowett DHowett changed the title Rejuvenate the color schemes page in the SUI to follow Win 11 style guidelines Rejuvenate the color schemes page Aug 31, 2022
@carlos-zamora carlos-zamora added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Needs-Second It's a PR that needs another sign-off labels Aug 31, 2022
@ghost
Copy link

ghost commented Aug 31, 2022

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.

// when the window has already been closed.. We feel this is okay though,
// because WE'RE ALREADY IN TEARDOWN! The app is exiting. We don't want a
// XAML bug here to create a crash bucket.
_app.UnhandledException([](auto&& /*sender*/, const UnhandledExceptionEventArgs& args) {
Copy link
Member

Choose a reason for hiding this comment

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

i didn't notice this sneak in. gosh!

@carlos-zamora
Copy link
Member

Huh, I guess the bot's broken today. Merging!

@carlos-zamora carlos-zamora merged commit 7076613 into main Aug 31, 2022
@carlos-zamora carlos-zamora deleted the dev/pabhoj/sui_schemes_page branch August 31, 2022 21:15
ghost pushed a commit that referenced this pull request Sep 1, 2022
## Summary of the Pull Request
Fix a bug where if you pressed the "Save" button, WT would crash. This was caused by adding the possibility that no color scheme is selected in the main page. With no "current scheme", attempting to get its "name" would cause a null ptr exception.

The fix is simple: just check if we actually have a current scheme.
Bonus points: if we don't have a current scheme, don't bother looking throught the color schemes for a match because we'll never find one.


## References
#13269 - Color Schemes Rejuv
@ghost
Copy link

ghost commented Sep 13, 2022

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings UI Anything specific to the SUI AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

People think the Color Scheme editor will change the current colors
5 participants