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

Style the button and tab view background to match the titlebar #1934

Merged
merged 3 commits into from Jul 12, 2019

Conversation

cinnamon-msft
Copy link
Contributor

Summary of the Pull Request

Added additional styling to the title bar so it looks cohesive and tweaked the + button so it's not so large.

References

PR Checklist

  • Closes UI: align design with other UWP apps #615
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

image

image

Validation Steps Performed

@DHowett-MSFT
Copy link
Contributor

nit: rename pull request to be in imperative form. Imagine that you're completing the sentence,

"[if merged,] this pull request will..."

"style the title bar"

@cinnamon-msft cinnamon-msft changed the title Styled title bar to be cohesive and shrunk + button style the title bar Jul 11, 2019
Background="{ThemeResource ApplicationPageBackgroundThemeBrush}"
HorizontalAlignment="Left"
Background="{ThemeResource SystemChromeLowColor}"
HorizontalAlignment="Left"
Copy link
Contributor

Choose a reason for hiding this comment

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

spacing issues

@@ -130,7 +130,10 @@
</StackPanel.Resources>

<Grid x:Name="Content"></Grid>
<Border Height="36.0" MinWidth="160.0" x:Name="DragBar" Background="{ThemeResource ApplicationPageBackgroundThemeBrush}" DoubleTapped="DragBar_DoubleTapped"/>
<Border Height="36.0"
MinWidth="160.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a loottt of spaces here. could you align the properties vertically after the space after the tag name?

Copy link
Member

Choose a reason for hiding this comment

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

(I think this is a tab/spaces thing)

@@ -31,7 +31,7 @@
</ResourceDictionary>
<ResourceDictionary x:Key="Dark">
<x:Double x:Key="CaptionButtonStrokeWidth">1.0</x:Double>
<StaticResource x:Key="CaptionButtonBackground" ResourceKey="SystemControlBackgroundAltHighBrush"/>
<StaticResource x:Key="CaptionButtonBackground" ResourceKey="SystemChromeLowColor"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

you might want to make these Transparent so the Row peeks through

@@ -18,7 +18,7 @@
<ResourceDictionary.ThemeDictionaries>
<ResourceDictionary x:Key="Light">
<x:Double x:Key="CaptionButtonStrokeWidth">1.0</x:Double>
<StaticResource x:Key="CaptionButtonBackground" ResourceKey="SystemControlBackgroundAltHighBrush"/>
<StaticResource x:Key="CaptionButtonBackground" ResourceKey="SystemChromeLowColor"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

(as below)

src/cascadia/TerminalApp/TerminalPage.xaml Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 11, 2019
@@ -23,21 +23,28 @@ the MIT License. See LICENSE in the project root for license information. -->
<ColumnDefinition Width="Auto"/>
</Grid.ColumnDefinitions>

<mux:TabView x:Name="TabView" Grid.Column="0" />
<mux:TabView x:Name="TabView" Grid.Column="0"
SelectionChanged="TabView_SelectionChanged" />
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this and the other selectionchanged thingos

@DHowett-MSFT DHowett-MSFT changed the title style the title bar Style the button and tab view background to match the titlebar Jul 12, 2019
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I pretty much have the same comments that Dustin has

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 12, 2019
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I think I'm cool with this. I can definitely merge this into #1948 without too much pain.

Aside, does this fix #1913?

@@ -130,7 +130,11 @@
</StackPanel.Resources>

<Grid x:Name="Content"></Grid>
<Border Height="36.0" MinWidth="160.0" x:Name="DragBar" Background="{ThemeResource ApplicationPageBackgroundThemeBrush}" DoubleTapped="DragBar_DoubleTapped"/>
<Border Height="36.0"
Copy link
Member

Choose a reason for hiding this comment

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

Why does GH think this line wrapped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added that in

@cinnamon-msft
Copy link
Contributor Author

I think I'm cool with this. I can definitely merge this into #1948 without too much pain.

Aside, does this fix #1913?

No I don't think so. When testing I was still experiencing the bug

@zadjii-msft
Copy link
Member

@cinnamon-msft cool, cool cool cool cool cool.

@DHowett-MSFT DHowett-MSFT merged commit b706b60 into master Jul 12, 2019
@DHowett-MSFT DHowett-MSFT deleted the cinnamon/button-styling branch July 12, 2019 22:07
mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 23, 2019
@ghost
Copy link

ghost commented Aug 3, 2019

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Aug 3, 2019
@brainplot
Copy link

I'm running version 0.3.2171.0 freshly downloaded from the Microsoft Store. I've set my theme to One Half Dark but the buttons and tabs remain white. Is there any additional settings I should add to my profiles.json?

terminal

@Stanzilla
Copy link
Contributor

You need to switch your Windows theme from light to dark in the settings app.

@brainplot
Copy link

Oh, I understand now. I prefer a light theme for the rest of the system. I guess I'll stick with the white borders for now. It'd be nice to be able to change this from the profiles.json for the Terminal app only.

@mdtauk
Copy link

mdtauk commented Aug 10, 2019

You can set the app to use dark themes in the settings

@DHowett-MSFT
Copy link
Contributor

Yeah, it’s called “requestedTheme”.

@brainplot
Copy link

Thank you! I was indeed going to ask what the option is :)

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.

UI: align design with other UWP apps
7 participants