added accent color to hamburger button #682

markti · 2016-12-03T13:44:12Z

I also changed the visual state for the hamburger button to add a semi-transparent white overlay onto the button to mimic the style of standard Windows 10 apps.

dnfclas · 2016-12-03T13:44:16Z

Hi @markti, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

ScottIsAFool · 2016-12-03T13:46:00Z

Microsoft.Toolkit.Uwp.UI.Controls/HamburgerMenu/HamburgerMenu.Properties.cs

@@ -111,6 +116,15 @@ public double CompactPaneLength
        /// <summary>
        /// Gets or sets the Brush to apply to the background of the Pane area of the control.
        /// </summary>
+        public Brush AccentColor


I would rather have this property called "HighlightColor" rather than AccentColor. AccentColor gives no clear indication what it's actually used for.

I would call it ButtonBackground

HighlightColor sounds good. Scratch that. I like AccentBrush. It is the Accent for the control itself. Or we could go with HamburgerButtonBackground and HighlightBrush. I hate the second option and would rather not have properties (and just allow user to restyle) over this option.
Third option: Have a defined brush style that could be changed. I like this idea the best and follows the core framework controls

Do we have any precedent set forth in other controls? So far there are several name candidates: AccentColor, HighlightColor, AccentBrush, ButtonBackground, HighlightBrush, HamburgerButtonBackground.

I'd like to use this property to set the brush for the selection indicator in the same way that it is done in the standard Windows 10 apps (News and Money) so this brush will eventually be used for more than just the button background. That's why I prefer a name that is not tied specifically to the control itself. In Windows 10 apps (News, Money) the pane background is independent of the button background. However, the button background is tied to the selection indicator accent. This is why I prefer AccentColor. 1) It recognizes the Hamburger has an AccentColor and a PaneColor. 2) The Accent Color affects multiple controls rendering, 3) It should not be a gradient or some other brush but a solid color.

A couple more examples, in 'Groove Music' and 'Movies & TV' the app does not have an AccentColor. Just a PaneBackground. The Back Button defaults to the system theme color.

News, Money, Sports each have an AccentColor (varies), PaneColor (#2B2B2B) and a Top Nav Color (#F2F2F2).

Instead of a property on the control itself, define a brush resource that is used for the background of the button. Based on your examples, you would want two brushes, one for the hamburger background and one for the selection brush.
Example:

<SolidColorBrush x:Key="HambugerMenuHamburgerBackgroundBrush" Color="#FFFFFF"/> <SolidColorBrush x:Key="HambugerMenuSelectionHighBrush" Color="#FFFFFF"/>

This is in line with what the core controls do for all of their brushes

I think this should be as easy to set by the developer. The Hamburger should, as part of it's defining characteristic as a control, have an Accent Color. This accent color should affect the button background as well as the visual selection indicator (vertical rectangle on the left-bound margin of the hamburger items....not yet implemented). I envision this property to control this Accent Color that will affect at least two (maybe more visual elements). Foreground is universal for text color, Background is universal for the controls background, in the case of the Hamburger Background affects the background color for the Hamburger's Content area. We (rightfully) have a Pane Background. The only thing missing is the Accent Color (or whatever we want to name it) that will control the button / visual selection indicator's background color as seen in the image below:

ScottIsAFool · 2016-12-03T13:47:32Z

Microsoft.Toolkit.Uwp.UI.Controls/HamburgerMenu/HamburgerMenu.xaml

-<ResourceDictionary xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
-                    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
-                    xmlns:local="using:Microsoft.Toolkit.Uwp.UI.Controls">
+<ResourceDictionary 


This xaml file has had too many unnecessary changes made, please use xaml styler (https://marketplace.visualstudio.com/items?itemName=NicoVermeir.XAMLStyler) to align the xaml properly.

What unnecessary changes? I added a control template and visual states for the hamburger button. There were some xmlns that were added too.

What do you mean "what unnecessary changes?" Look at the diff! If all you did to the file was add the template and visual state, that should be the only changes I see in that file. They aren't, you've changed the style of the xaml.

Yep too many unnecessary changed here

Sorry, I was accessing from my W10 mobile phone while out with family and couldn't see the diff very well. It looks like you aren't a fan of the indentation that I made to the Resource Dictionary xmlns declarations? Is that the concern? Or is the concern that I added two more xmlns declarations and a ignorable property ("d", "mc", and "mc:Ignorable")?

Agreed, please revert these changes

ok removed extra xmlns and white space formatting

markti · 2016-12-03T15:17:34Z

Can't get the keyboard to come up on windows mobile 10 edge when replying so I'll comment in here about the property name. @skendrot suggested AccentColor and I like it over HighlightColor because it will be used for more things than just highlighting. It is intended to be used to control the color that is used in the hamburger button and item seldcttion similar to the accent color used in the news, money apps for w10. News has a Indian red and money has a green accent color that permeates the app. Hence the term accent color.

timheuer · 2016-12-03T18:55:09Z

Microsoft.Toolkit.Uwp.UI.Controls/HamburgerMenu/HamburgerMenu.xaml

+                            Padding="0"
+                            VerticalAlignment="Top"
+                            AutomationProperties.Name="Main button"
+                            Background="{TemplateBinding AccentColor}"


Where is the default being set for this new TemplateBinding?

Hi @timheuer! I followed the practices used for the PaneBackground Property. There was no default set. Not in XAML nor in the Dependency property definition. If the property is unset, I expect that it will default to null which should resolve to Transparent brush being applied. This is what I observed during my manual testing.

I set the default to be the system accent color in the Style in my latest commit.

skendrot · 2016-12-05T15:26:53Z

Microsoft.Toolkit.Uwp.UI.Controls/HamburgerMenu/HamburgerMenu.Properties.cs

@@ -45,6 +45,11 @@ public partial class HamburgerMenu
        /// <summary>
        /// Identifies the <see cref="PaneBackground"/> dependency property.
        /// </summary>
+        public static readonly DependencyProperty AccentColorProperty = DependencyProperty.Register(nameof(AccentColor), typeof(Brush), typeof(HamburgerMenu), new PropertyMetadata(null));


This should probably default to the system accent color. It should default here, but in the xaml definition of the control.

I will set the default to be {ThemeResource SystemAccentColor}. All agree?

Yep, that's fine.

skendrot · 2016-12-05T15:27:32Z

Microsoft.Toolkit.Uwp.UI.Controls/HamburgerMenu/HamburgerMenu.Properties.cs

@@ -111,6 +116,15 @@ public double CompactPaneLength
        /// <summary>
        /// Gets or sets the Brush to apply to the background of the Pane area of the control.


XML comments needs to be fixed

ok will fix.

skendrot · 2016-12-05T15:27:42Z

Microsoft.Toolkit.Uwp.UI.Controls/HamburgerMenu/HamburgerMenu.Properties.cs

@@ -45,6 +45,11 @@ public partial class HamburgerMenu
        /// <summary>
        /// Identifies the <see cref="PaneBackground"/> dependency property.


Need to fix XML comment

skendrot · 2016-12-05T15:28:22Z

Microsoft.Toolkit.Uwp.UI.Controls/HamburgerMenu/HamburgerMenu.xaml

-<ResourceDictionary xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
-                    xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
-                    xmlns:local="using:Microsoft.Toolkit.Uwp.UI.Controls">
+<ResourceDictionary 


Agreed, please revert these changes

ScottIsAFool · 2016-12-05T20:39:57Z

@markti you still need to sign the CLA before this can be merged (once approved)

markti · 2016-12-05T21:24:36Z

@ScottIsAFool, I signed the CLA over the weekend. Not sure what else I need to do or if there is a delay....

ScottIsAFool · 2016-12-06T14:40:32Z

@markti there shouldn't be a delay. @deltakosh is there something you can check?

deltakosh · 2016-12-06T14:41:36Z

I confirm this is immediate

deltakosh · 2016-12-06T14:41:56Z

Can you try to sign it again?

markti · 2016-12-11T20:40:10Z

I will sign it again.

dnfclas · 2016-12-11T20:51:24Z

@markti, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

markti · 2016-12-11T20:59:55Z

Maybe I didn't do the DocuSign part. :) Anyways, signed it and committed some changes to:

clean up code comments / dependency property documentation
removed unnecessary xmlns references and whitespace formatting in HamburgerMenu.xaml
set the default value to be the System Theme Color

markti · 2016-12-21T04:44:39Z

is there anything additional changes needed by any of the reviewers?

deltakosh · 2016-12-21T17:20:53Z

I was expecting the highlight as well but could be part of another PR

markti · 2016-12-21T19:46:36Z

@deltakosh, ok I can add that too...

deltakosh · 2017-01-03T17:33:40Z

Hello some news here? :) Happy new year by the way!

deltakosh · 2017-01-12T23:22:48Z

Hello? We are close to merge this one :) do you need help to add the highlight ?

deltakosh · 2017-01-23T17:14:30Z

Moving it to 1.4

skendrot · 2017-02-13T04:14:21Z

@markti How are you coming on this? Need help finishing it?

deltakosh · 2017-02-15T16:22:52Z

Hello we won't add more code to Hamburger Menu because the XAML team will soon officially provide one : https://developer.microsoft.com/en-us/windows/platform/features/navigationview/?q=navigation%20view

I really want to thank you for the time spent on this PR but I do not want to waste your time on a control we will deprecate in favor of XAML one.

added accent color

7b72447

dnfclas added the cla-required label Dec 3, 2016

ScottIsAFool reviewed Dec 3, 2016

View reviewed changes

timheuer reviewed Dec 3, 2016

View reviewed changes

skendrot suggested changes Dec 5, 2016

View reviewed changes

dnfclas added cla-signed and removed cla-required labels Dec 11, 2016

CommunityToolkit#682 review changes

95c03a8

skendrot mentioned this pull request Dec 12, 2016

Change color of Hamburger Button separate from the hamburger pane #681

Closed

deltakosh mentioned this pull request Jan 3, 2017

Added HamburgerBackground property #755

Closed

deltakosh added this to the v1.4 milestone Jan 23, 2017

pedrolamas approved these changes Feb 3, 2017

View reviewed changes

skendrot mentioned this pull request Feb 7, 2017

How to change Background color of SelectedItem in HamburgerMenu? #886

Closed

deltakosh closed this Feb 15, 2017

		@@ -111,6 +116,15 @@ public double CompactPaneLength
		/// <summary>
		/// Gets or sets the Brush to apply to the background of the Pane area of the control.

		@@ -45,6 +45,11 @@ public partial class HamburgerMenu
		/// <summary>
		/// Identifies the <see cref="PaneBackground"/> dependency property.

added accent color to hamburger button #682

added accent color to hamburger button #682

Conversation

markti commented Dec 3, 2016

dnfclas commented Dec 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skendrot Dec 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skendrot Dec 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markti commented Dec 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScottIsAFool commented Dec 5, 2016

markti commented Dec 5, 2016

ScottIsAFool commented Dec 6, 2016

deltakosh commented Dec 6, 2016

deltakosh commented Dec 6, 2016

markti commented Dec 11, 2016

dnfclas commented Dec 11, 2016

markti commented Dec 11, 2016

markti commented Dec 21, 2016

deltakosh commented Dec 21, 2016

markti commented Dec 21, 2016

deltakosh commented Jan 3, 2017

deltakosh commented Jan 12, 2017

deltakosh commented Jan 23, 2017

skendrot commented Feb 13, 2017

deltakosh commented Feb 15, 2017

skendrot Dec 5, 2016 •

edited

skendrot Dec 5, 2016 •

edited