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

Added requestedTheme option into terminal settings #710

Merged
6 commits merged into from
May 20, 2019
Merged

Added requestedTheme option into terminal settings #710

6 commits merged into from
May 20, 2019

Conversation

Gabboxl
Copy link
Contributor

@Gabboxl Gabboxl commented May 11, 2019

Added the possibility to change the theme (dark/light/default) from the terminal settings

@msftclas
Copy link

msftclas commented May 11, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

@mdtauk
Copy link

mdtauk commented May 11, 2019

The most common scenarios should be there in the GUI, with a button in the settings screen to open the commented JSON file with advanced settings

@DHowett-MSFT
Copy link
Contributor

This change just exposes that setting in the default profiles.json, right? Without this change, if you add "requestedTheme": ... it will still work, you just won't know the setting exists.

Gusted
Gusted approved these changes May 11, 2019
View reviewed changes
Copy link

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

Good thinking to add this as an option!

miniksa
miniksa previously requested changes May 16, 2019
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

I have a tiny nit pick newline comment, but I also want an answer to @DHowett-MSFT's question before I sign.

src/cascadia/TerminalApp/GlobalAppSettings.cpp Show resolved Hide resolved
@Gabboxl
Copy link
Contributor Author

Gabboxl commented May 17, 2019

@DHowett-MSFT yes, this change just exposes the setting to change the theme in the json file.

DHowett-MSFT
DHowett-MSFT suggested changes May 17, 2019
View reviewed changes
src/cascadia/TerminalApp/CascadiaSettings.cpp Outdated
@@ -154,6 +154,8 @@ void CascadiaSettings::_CreateDefaultProfiles()
_profiles.emplace_back(cmdProfile);

_globals.SetDefaultProfile(powershellProfile.GetGuid());

_globals.SetRequestedTheme(winrt::Windows::UI::Xaml::ElementTheme::Default);
Copy link
Contributor

Choose a reason for hiding this comment

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

CreateDefaultProfiles isn't a great place for this .. is there something more generic or less default-profile-related to put it?

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 17, 2019
@DHowett-MSFT
Copy link
Contributor

That makes sense. Thanks!

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 18, 2019
@Gabboxl
Copy link
Contributor Author

Gabboxl commented May 18, 2019

I found what was blocking the serialization of the requestedTheme value in the json file. Now it works.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 20, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 20, 2019
@DHowett
fix indentation part 2
6355610 
Co-Authored-By: Dustin L. Howett (MSFT) <duhowett@microsoft.com>
@DHowett-MSFT DHowett-MSFT added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 20, 2019
@ghost
Copy link

ghost commented May 20, 2019

Hello @DHowett-MSFT!

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 and give me an instruction to get started! Learn more here.

@DHowett-MSFT DHowett-MSFT dismissed miniksa’s stale review May 20, 2019 19:24

As far as I can tell, Michael's only blocker was getting an answer to my question. As it's been answered satisfactorily, I'm clearing his block.

@zadjii-msft zadjii-msft mentioned this pull request May 20, 2019
Merged
@ghost ghost merged commit 0060614 into microsoft:master May 20, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpotter2 gpotter2 gpotter2 left review comments

@Gusted Gusted Gusted approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

@miniksa miniksa miniksa left review comments

Assignees
No one assigned
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Gabboxl @msftclas @mdtauk @DHowett-MSFT @gpotter2 @miniksa @zadjii-msft @Gusted