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

Replace caption buttons with ones that look close to correct #1627

Merged
merged 3 commits into from
Jul 1, 2019

Conversation

DHowett-MSFT
Copy link
Contributor

Summary of the Pull Request

This pull request contains a bunch of XAML that makes our caption buttons look like this:

state image
normal image
hovered image
clicked image
maximized image

instead of like the characters _, - and X from Segoe UI.

References

Part of #1625.

PR Checklist

  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed/N/A
  • I've discussed this with core contributors already.

@DHowett-MSFT DHowett-MSFT requested a review from ocalvo June 26, 2019 00:13
@ocalvo
Copy link
Contributor

ocalvo commented Jun 26, 2019

I am not an expert on Visual State manager, Alan Wu is our expert.

@ocalvo
Copy link
Contributor

ocalvo commented Jun 27, 2019

@jevansaks could someone from control take a look at the VSM code.
Thanks.

@fcharlie
Copy link
Contributor

@DHowett-MSFT I am waiting for the merger of this PR. Yesterday I built Windows Terminal, it is really ugly, the window is not right angle, the black edge of the window (dark theme) makes me difficult to adapt.

@DHowett-MSFT
Copy link
Contributor Author

High contrast mode, X, hovered:

image

@DHowett-MSFT
Copy link
Contributor Author

@fcharlie follow #1625 for progress on this

@fcharlie
Copy link
Contributor

@DHowett-MSFT Great!!!

@jevansaks
Copy link
Member

jevansaks commented Jun 28, 2019

High contrast mode, X, hovered:

image

Are you in high contrast black? I think buttons usually hover into light blue unless you've customized the colors. I'd expect:

image

If you've selected High Contrast #1 then I think that's right. Thanks!

@DHowett-MSFT
Copy link
Contributor Author

Oh, that’s weird. I actually just chose to enable high contrast with the keyboard shortcut. That said, I’m using the recommended high contrast brushes so it should be using high cyan for the other HC theme.

Copy link
Contributor

@ocalvo ocalvo left a comment

Choose a reason for hiding this comment

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

I am approving, but please wait for @jevansaks approval/suggestions.

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'd say I fully understand like 85% of this, but it looks good to me :shipit:

<x:String x:Key="CaptionButtonPath"></x:String>
<x:String x:Key="CaptionButtonPathWindowMaximized"></x:String>

<Style x:Key="CaptionButton" TargetType="Button">
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 it might be helpful to add some comments explaining how this works for people who are less fluent in XAML (like me)

@DHowett-MSFT DHowett-MSFT merged commit 171c370 into master Jul 1, 2019
@DHowett-MSFT DHowett-MSFT deleted the dev/duhowett/minmaxcloseme branch July 1, 2019 21:39
@zadjii-msft
Copy link
Member

You're the man

mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 2, 2019
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.

6 participants