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

Add reveal style to Settings button in NavigationView in top mode,Closes #942 #1236

Merged
merged 3 commits into from Aug 31, 2019
Merged

Add reveal style to Settings button in NavigationView in top mode,Closes #942 #1236

merged 3 commits into from Aug 31, 2019

Conversation

marcelwgn
Copy link
Contributor

Summary

Added reveal style to settings button in NavigationView when it is in top mode.

Change

Added the item "Border x:Name=RevealBorder" to the template of the settings button in top mode

Testing

Tested by starting MUXControlsTestApp and navigating through following test pages:

  • NavigationView
    • NavigationView TopNav test
    • Top NavigationViewTest
    • NavigationView ItemTemplate test

Motivation

Fixes #942.

@marcelwgn marcelwgn requested a review from a team as a code owner August 29, 2019 14:31
@jevansaks
Copy link
Member

Thanks for this contribution, @chingucoding! We'll take a look soon.

YuliKl
YuliKl reviewed Aug 29, 2019
View reviewed changes
dev/NavigationView/NavigationView_rs1_themeresources.xaml Outdated
x:Name="RevealBorder"
BorderBrush="{TemplateBinding BorderBrush}"
BorderThickness="{TemplateBinding BorderThickness}"
contract4NotPresent:Visibility="Collapsed" >
Copy link

Choose a reason for hiding this comment

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

I don't think you need this contract check. I think the value of BorderBrush (NavigationViewItemBorderBrush) is already doing the right thing depending on which version of the OS the code is running on. NavigationViewItemBorderBrush sometimes maps to SystemControlTransparentBrush, and other times, when reveal is available, to SystemControlBackgroundTransparentRevealBorderBrush.

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 thought about that too. This Border is also used in the "MUX_NavigationViewItemPresenterStyleWhenOnLeftPane" style . Should we remove the contract4NotPresent for that Border element too?

Copy link

Choose a reason for hiding this comment

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

@kaiguo what would you recommend?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think it's safe to remove them altogether.

@chingucoding You probably need to update reveal states and maybe border brushes too in the visual state setters. Take a look at "MUX_NavigationViewItemPresenterStyleWhenOnLeftPane"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chingucoding You probably need to update reveal states and maybe border brushes too in the visual state setters. Take a look at "MUX_NavigationViewItemPresenterStyleWhenOnLeftPane"

I don't think this is necessary since the reveal works just as expected:

Dark theme Light theme
navtop-settingsbutton-reveal-dark navtop-settingsbutton-reveal-light

Copy link
Contributor

Choose a reason for hiding this comment

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

ezgif-2-7690b84940d6

This is very subtle but it's not only the approaching animation, there are also hover and pressed animations, which requires setting reveal brush states.

https://docs.microsoft.com/en-us/windows/uwp/design/style/reveal#using-the-control-template-to-add-reveal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed now.
@kaiguo Thank you for the pointing out the right places to fix!

Copy link
Contributor

Choose a reason for hiding this comment

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

@chingucoding thanks for the fix!

There's another related issue, you probably have noticed, the items on top nav don't have reveal either. Would you like to contribute a fix for that too 😊?

20190831113543

Copy link
Contributor Author

@marcelwgn marcelwgn Aug 31, 2019 •
edited

Choose a reason for hiding this comment

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

@kaiguo Of course! Should the reveal be the same as for the settings button? Or should the reveal be different for those items (since the also have a different hover visual as the buttons)?

@jevansaks
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jevansaks jevansaks added this to the WinUI 2.2 milestone Aug 30, 2019
@kaiguo
Copy link
Contributor

kaiguo commented Aug 30, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kaiguo kaiguo added the auto merge This PR will be merged once all checks pass label Aug 31, 2019
@kaiguo kaiguo force-pushed the navigationview-topmode-settings-noreveal branch from 36cee22 to 434a107 Compare August 31, 2019 05:42
@kaiguo
Copy link
Contributor

kaiguo commented Aug 31, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@msft-github-bot msft-github-bot merged commit e587671 into microsoft:master Aug 31, 2019
@marcelwgn marcelwgn mentioned this pull request Sep 1, 2019
Merged
@jevansaks jevansaks added needs-cherrypicktorelease PR tagged for cherry-pick to the current release branch (but not yet picked) release note PR that we want to call out in the next release summary labels Sep 10, 2019
@msft-github-bot
Copy link
Collaborator

🎉Microsoft.UI.Xaml v2.3.190909001-prerelease has been released which incorporates this pull request.:tada:

Handy links:

@msft-github-bot msft-github-bot mentioned this pull request Sep 10, 2019
Closed
@jevansaks jevansaks removed the needs-cherrypicktorelease PR tagged for cherry-pick to the current release branch (but not yet picked) label Sep 12, 2019
jevansaks pushed a commit that referenced this pull request Sep 12, 2019
@marcelwgn @jevansaks
Add reveal style to Settings button in NavigationView in top mode,Closes
52f5bb1 
 #942 (#1236)

* Add reveal style to Settings button in NavigationView in top mode,Closes #942

* Remove unnecessary contract check

* Add missing reveal state setters
@marcelwgn marcelwgn deleted the navigationview-topmode-settings-noreveal branch September 13, 2019 11:50
@msft-github-bot
Copy link
Collaborator

🎉Microsoft.UI.Xaml v2.2.190917002 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
Reviewers

@kaiguo kaiguo kaiguo approved these changes

@YuliKl YuliKl YuliKl left review comments

Assignees

@kaiguo kaiguo

Labels
auto merge This PR will be merged once all checks pass release note PR that we want to call out in the next release summary
Projects
None yet
Milestone
WinUI 2.2
Development

Successfully merging this pull request may close these issues.

NavigationView: settings button not using fluent
5 participants
@marcelwgn @jevansaks @kaiguo @msft-github-bot @YuliKl