Windowscommunitytoolkit: Change color of Hamburger Button separate from the hamburger pane

[markti]

There is a dependency property called PaneBackground that allows you to specify a brush for the fill color of the pane and hamburger button. I would like to see a separate dependency property to set the color of the hamburger button separately from the pane. Like the News, Money and other standard windows apps, I want my pane to be "#FF2b2b2b" but I want the button to be "CornflowerBlue". should be an easy fix.

[deltakosh]

Hello you can use the hamburgerMenu.HamburgerMenuTemplate

Here is the default value: https://github.com/Microsoft/UWPCommunityToolkit/blob/dev/Microsoft.Toolkit.Uwp.UI.Controls/HamburgerMenu/HamburgerMenu.xaml#L6

[markti]

It should be easier. Why not poke a hole by just adding a DP for button background so I don't have to override the entire control template?

[deltakosh]

Then we also need one for text font, color, size, etc... :)

[markti]

I don't think so. The hamburger button doesn't have text. it is just the hamburger symbol (the three lines). So there is no text. The scenario I am talking about is extremely common. whereas changing the text color size as you mentioned are not very common scenarios. The hamburger button will always be a different color than the pane (based on Windows 10 design guidelines). It's extremely simple to add a dependency property to change the color. I could've done it by now I don't know why we need to debate it.

[deltakosh]

We need to debate because no one owns the truth. The icon is displaedy with a FontIcon so you can change the font, the size and the color.

While I'm ok to add this property, I just want to ensure that we do not add too much properties when not absolutely required.

I'm ok for this one because (as you mentioned) we already provide the PaneBackgroundColor

[skendrot]

This seems similar to #573

[markti]

Agree on not adding unnecessary DPs as it will be wasted effort. If somebody wants to reskin the control the controlTemplate is the right place to do it. However, adding DPs to support commonly used scenarios should be done. I think achieving parity with the design guidelines would be the goal. Therefore, I think it's a good trade off between a single DP and getting the hamburger to look almost like the Windows 10 standard apps.

[markti]

I hope we can agree on the following statement (and general goal), "as a uwp toolkit user, I don't want to drop into the control template to perform extremely common scenarios to make my app look like a standard UWP app."

[markti]

I do still think #573 should be an open improvements item. I would work on it, just so I could us it in my own apps. I don't see how lack of interest should be equated to making something worthwhile. I mean we are UWP developers ;) However, I still think the button background should be accessible from DP and should stand as its own issue (separate from #573 as that one entails modifying the default control template rather than opening up a DP to aid developer productivity)

[deltakosh]

Agree on the backgroundCOlor for the button, you can proceed :)

[skendrot]

I agree that the default style for the hamburger menu should match that of the official windows apps

[skendrot]

I would recommend an AccentColor or similar that the hamburger button would use along with a small side color for the currently selected menu item. By default this would use the title bar theme color if it's accessible

[deltakosh]

just think we cannot introduce break changes :)

[skendrot]

Modifying the style isn't a breaking change. (So long as you're not enforcing controls/content be present in OnApplyTemplate or elsewhere)

[deltakosh]

Visually it is

[markti]

checked in :)

[markti]

shouldn't break users that are already using the PaneBackground. If accent color is not set then the button remains transparent (as it is in the original control template)

[markti]

I called the DP 'AccentColor' as @skendrot suggested. We can use this to add a selection rectangle of the same color to further align the hamburger control with UWP standard apps.

[ScottIsAFool]

Looking at this now (I didn't see this thread when it was being discussed), I don't actually think we should have an additional property at all. I agree with @skendrot that the change should be made in the default template and we use the SystemAccentColor resource. If the app is wanting a custom Accent colour that's different from the user's own choice, the SystemAccentColor resource is usually overridden anyway.

[deltakosh]

Agree. I do not like the name. We should name if ButtonBackground (like PaneBackground).

[markti]

I don't want to put words in @skendrot 's mouth but he appeared to support the addition of a property called AccentColor in addition to making changes to the default control template of the Hamburger control to make the default implementation reflect the UI guidelines as implemented in the standard W10 apps (News, Money, etc.) @deltakosh was originally against it. I think it is unreasonable to require developers to override the Control Template simply to change the button color. This should be a simply thing to do. based on the UWP UI Guidelines as implemented in the standard W10 apps, the button should use an accent color tied to the branding of the app (not necessarily the system accent color) and the pane background should differ (the standard W10 apps use "# FF2B2B2B").

[deltakosh]

We cannot call it as it does not align with general naming for control. We should call it ButtonBackground if it only affect button background

[lukasf]

I agree that the control should look and behave like in the standard windows apps, out of the box, so I am surprised to see that #573 was closed. It should not be neccessary to re-style the control, just to achieve the standard look and feel. In fact, nothing should be required to get the standard look and feel.

For the DP naming, ButtonBackground does not fully fit I think. The color would be used for both the hamburger button background, plus the selection highlight, but not for the background of the other (not selected) buttons. AccentColor would fit better in that sense. For performance reasons, this should be a brush, not a color (all standard control "color" DPs are brushes). So maybe name it something like AccentBrush, ButtonAccent, ButtonAccentBrush, or even just Accent?

[deltakosh]

I'm not a big fan of AccentBrush (never seen such a property in any official control). If we want we can change the control to use the ThemeAccentColor for button background and highlights. NO need for a DP then.
Furthermore we must be cautious when changing a published control as it will be a (visual) breaking change. However in this specific case, I guess that being better aligned to the standard windows apps is a good reason to do it.

[markti]

The developer needs to be able to change the Accent Color of the hamburger control. . Green, Purple, Red, we can make it default to the system theme color but the developer needs a DP to override the color.

[markti]

Visual breaking changes aren't breaking changes. Besides, the way I implemented the AccentColor DP it isn't even a "breaking" visual change as I defaulted the property to be Transparent so it will look EXACTLY like it does today. The Accent Color just allows us to take one step closer to the above picture. We can change the background color of the button. The next step would be to add the selection visual to use the same AccentColor property. Again, if the developer (or existing users of the control) get the latest version use it then it will be transparent and will have NO affect on there code. This would be a visual _improvement_. If we ever make a "breaking" visual change (misnomer if you ask me), if people want the old styles we can release the control templates for them. But I agree with @lukasf that we should be enabling the kind of experience out-of-the-box in the above picture. We shouldn't require developers to reskin it and we shouldn't use the old (inferior) style / control template as an excuse NOT to improve / implement the standard way.

[deltakosh]

Ok guys you convinced me:)
But please make the change so that the highlights is also in the PR.
And keeping the value transparent by default is great to me as I do not want this behavior in my apps. But I would love to get more thoughts on this one.

Once again I'm ok to change the control but in the right (and community validated way)

[markti]

I gotta somehow get the CLA signed. I'm about to check in the fixes that were requested in the feedback to my original pull request. .

I think we still have to figure out what to call the property! lol

[deltakosh]

I'm for HighlightBrush :)

[markti]

We got one for HighlightBrush. I'm for AccentColor :) Any other votes?

[lukasf]

I like HighlightBrush as well.

The current PR does not contain the selection highlight? That's not so good. Users would end up with two different colors, unable to change the selection color. At least in my opinion, a PR should contain a full solution. So at least selection color should be included. Best would be to also include selection style (small bar instead of full background).

I am not 100% sure how to best achieve the selection color, because it must be made available on the item containers. One way might be using an attached property for the ListViewItems, bind it to HighlightBrush in PrepareContainerForItemOverride and then use that attached property in the ItemContainerStyle's Template for binding to the SelectedBackground. But this would require subclassing the ListViews. Maybe there is a better way.

[deltakosh]

I'm with you. I also asked for the PR to include selection highlight

[skendrot]

I vote for no new property. Just use a defined brush resource that can be overridden. Think about the core controls, they don't have properties for every thing that has a brush. They do use defined brush resources.
Examples:
ToggleBrush:

<Rectangle x:Name="SwitchKnobBounds" 
           Fill="{ThemeResource SystemControlHighlightAccentBrush}" Height="20" Opacity="0" RadiusY="10" Grid.Row="2" RadiusX="10" 
           Stroke="{ThemeResource SystemControlHighlightAltBaseHighBrush}" 
           StrokeThickness="{ThemeResource ToggleSwitchOnStrokeThickness}" Width="44"/>
...
...
<Ellipse x:Name="SwitchKnobOn" 
         Fill="{ThemeResource SystemControlHighlightAltChromeWhiteBrush}" Height="10" Opacity="0" Width="10"/>
<Ellipse x:Name="SwitchKnobOff" 
         Fill="{ThemeResource SystemControlForegroundBaseMediumHighBrush}" Height="10" Width="10"/>

CheckBox:

<ObjectAnimationUsingKeyFrames Storyboard.TargetProperty="Fill" Storyboard.TargetName="NormalRectangle">
    <DiscreteObjectKeyFrame KeyTime="0" Value="{ThemeResource SystemControlHighlightAccentBrush}"/>
</ObjectAnimationUsingKeyFrames>

ComboBox:

<Setter Property="Foreground" Value="{ThemeResource SystemControlForegroundBaseHighBrush}"/>
<Setter Property="Background" Value="{ThemeResource SystemControlBackgroundAltMediumLowBrush}"/>
<Setter Property="BorderBrush" Value="{ThemeResource SystemControlForegroundBaseMediumLowBrush}"/>

So. Instead of adding a new property, just use the theme resource SystemControlHighlightAccentBrush

[deltakosh]

At last!! Someone who agrees with me. Thanks @skendrot !

[skendrot]

I've been saying this, but it was hidden under a review.

[markti]

@lukasf It provides incremental value. There's no reason why we wouldn't release it now. Users can take advantage of the limited functionality now (I know I will in my UWP apps). I only implemented what I had time for. But honestly, all this debate over a simple (IMHO value add) change. I've probably spent more time navigating the politics of this project than actually adding features!

[deltakosh]

Thake it for a big interest from the community. Furthermore we cannot ship features without thinking about implication.
The current version can be updated by changing the template. You want to provide simplification and this is cool. But we need to do it in the right way :)
Let's be honest we are close right now. Like @skendrot I think we should just change the default color of button's background. And I'm with @lukasf for also using this PR to add highlights
This is not politics. This is constructive discussions between developers working on the same project

[markti]

@skendrot defined brushes to me are a rather hidden method for accomplishing the same thing. If we want to make it easy for developers to set and change the accent color on the HamburgerMenu then it should be a DP. As I've argued before, the Hamburger Menu is different from other controls. It is one of the few controls where the System Accent Color or the App's Accent Color plays an important role in the branding of the user experience of the app. That's why I think the Hamburger Menu is distinct from other controls that don't have "properties for everything that has a brush". The Accent Color and the Hamburger Menu are key bedfellows. The Accent Color as it relates to the Hamburger control can't be considered "everything with a brush". It's a key and differentiating aspect of the control itself. I was shocked that the Hamburger control didn't have any way to do this without reskinning the whole control because I was building apps and I wanted to make them look like the UWP W10 apps but couldn't with out of the box features of the control. I'm feeling a bit like a broken record so I'll stop. :)

[deltakosh]

Don't feel disappointed. No one is completely convinced about a specific option.
Both sides have interesting arguments

[markti]

@deltakosh I have hope. :) I guess, this being my first dabbling in an open source project, I naively assumed it was more code slinging than talking :) That's not a knock on anybody--just my own personal lack of open source experience. It's hard when we're not in a room together, face to face. I'm sure we'd be bang through this discussion in an hour if that were the case! :)

[markti]

@deltakosh And thanks for the kind words and encouragement. 👍

[deltakosh]

I would have love to discuss this over a whiteboard. I don't have the holy truth and I have mixed feelings about this DP.
Let's wait one day or two to see if more people want to share their opinions

In the meantime I think we agreed on the need for the highlights . To me the only "pain" point is about the DP. And for some reasons i think it could be more obvious that the DP is required if this affect the background AND the highlights

[markti]

@deltakosh Agreed on all counts. I wanna do the selection indicator highlights, just haven't had the time. I thought I could lay the foundation with the AccentColor DP and then add the selection indicator highlights in a future pull request. :)

[deltakosh]

No rush :)

[skendrot]

@markti I believe your comments are the exact reason to use the SystemControlHighlightAccentBrush.

the Hamburger Menu is different from other controls. It is one of the few controls where the System Accent Color or the App's Accent Color plays an important role in the branding of the user experience of the app

This isn't unique to the HamburgerMenu. Many controls use the accent color. I gave two examples of checkbox and toggleswitch

The Accent Color and the Hamburger Menu are key bedfellows

Exactly why it should use the predefined accent brush. If users want to rebrand their app, changing the SystemControlHighlightAccentBrush is one of the first steps to rebrand multiple controls at once

If we want to make it easy for developers to set and change the accent color on the HamburgerMenu then it should be a DP

While this is the "Community Toolkit" it is a Microsoft product. As such we should probably adhere to the standards set forth by the XAML framework team. (This is my personal opinion, not necessarily the stance of the toolkit)

[skendrot]

I finished the previous post before I was actually finished:
The goal is to make it easier. But that doesn't mean everything is set through a DP. I'm personally against adding DP everywhere just to make one thing easier. Having the button and highlight of selected view use the default accent brush is making it easier. Following the standards set by the framework team and requiring changing one system brush that affects multiple controls is making it easier. It is not easier to change a property for control a, and control b, and control c, and control d in order to change the highlight color for those controls

[hermitdave]

I agree that we shouldn't add DP for Hamburger background / selection. All native controls use Accent Color / Brush and devs can override it if needed. For such specific requirement one should modify the raw source

[CabuxaMapache]

For me, the better approach would be add a DP with a defautl value set to default accent brush. This way is easy for me to change color and allows me to personalize if I want to.

[lukasf]

I somehow agree on both sides. It is true that a lot of stock controls use the system accent color, and the easiest way to switch all of them to a custom color is to just override that brush in the app resources. It is just one entry in the app xaml and all controls use the new accent color.

But on the other hand, I find it pretty unfortunate that the accent color, which is very prominent in some controls, cannot be changed by a DP, while foreground and background and often even others are available. The problem is that the technique of overriding the accent brush only works for static colors. But what if you want to change the accent color on the fly, based on user selection, or maybe switch to red in an error situation? You are out of luck or you have a lot of work. It would be a lot better if all stock controls would provide a DP to change accent/highlight colors, without having to re-skin the whole control. It is actually one of the biggest complaints about XAML/UWP that even for simple tasks you have to re-template whole controls, because there are no DPs available.

Due to that, I'd personally vote for adding a DP here, even if it will only help with this control and not with the stock controls. The main goal of the toolkit is to simplify tasks for developers. I do not feel confused when I see a HighlightBrush on a control - I'd rather think, cool, it lets me change the highlight color easily, unlike those stock controls.

@markti I also recently added a feature here for the first time, and found myself spending >90% of time writing and discussing. That can be a bit disappointing. But on the other hand it is true, if we would add every singe idea someone comes up with, the toolkit would soon be a bloated mess. There have to be discussions which features we really want to have in, and these discussions tend to be lengthy, with all the different views and ideas (plus people commenting delayed, from different time zones).

[deltakosh]

This is a really good feedback about stock controls.
Ping @timheuer for info

[deltakosh]

This discussion clearly identifies a potential need.
But so far (if we let the simplification aside) I'm not convinced by the "I want to change accent dynamically" argument. Is this a common use case? As you mention it is easy to force a specific accent color in the styles

[markti]

@lukasf Interesting angle. I find DPs much easier and friendlier to work with than static brushes embedded god knows where. The DP also gives the flexability to set the Accent Color of the Hamburger to be the System / App Theme Color or not very easily. I would say, some vanilla apps I want to use the System Theme. Sometimes I want to use the App Theme. Sometimes I want the Hamburger Accent to be it's own thing. And as @lukasf pointed out, sometimes I may want the Accent Color to change dynamically based on user identity / context / etc. The DP provides the greatest flexibility to accomplish all these use cases with the minimal amount of work.

[markti]

@lukasf Thanks for the kind words and encouragement. I haven't been scared off quite yet :)

[skendrot]

Here's what it boils down to for me: If you want to retheme, you must override the SystemControlHighlightAccentBrush. Why require another thing to change? Requiring to change two things instead of one isn't making it easier.
Also, changing theme styles at runtime is possible

[lukasf]

@skendrot Adding a DP in addition to using the accent brush by default will not require two things to change the accent. You could still change all accent colors by just overriding the brush. The DP would only add a different (and more flexible) way to achieve this, which could be used instead of overriding the brush.

Also, changing theme styles at runtime is possible

Can you elaborate on that? StaticResource bindings will never update, ThemeResource bindings will only update on AppTheme changes (toggle light and dark theme). But none of these would pickup a dynamic change of the accent brush in the resource dictionary during runtime. So I do not see a way how I can dynamically override the system accent brush to a different brush, based on some condition in my code.

@deltakosh I had cases where I would have liked to change the accent color, e.g. on a slider when a certain threshold is overrridden or a toggle button when a certain condition is met. I also have an app where users can change the accent color, and it was a whole lot of work to get this running for all controls. This could have been much easier when there were DPs for it. I do not know how common this really is. But styling and colors are a very important aspect of app development, and I think it is not very well covered by the current UWP controls. I mean, even for the accent override, how should a novice app developer ever come up with this idea? They would look for some DP to change, and give up eventually because there is no such thing. Overriding brushes feels more like a hack and it is definitely far from intuitive and very poorly documented.

[deltakosh]

This makes sense to me and I'm starting considering changing my vote :)

[CabuxaMapache]

I have a question related with this: How can I change style of selected item? I want to highlight selected item like MSN apps!
Merry Christmas everybody!

[skendrot]

@CabuxaMapache The goal with this issue is that the selected item will have that highlight on the side as well

[CabuxaMapache]

Ok, waiting for the next release then, thank you!

[deltakosh]

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.

[imagin-tech]

Perhaps I'm missing something here, but there doesn't appear to be a way to change the background color of the button itself; I am not talking about the FontIcon element, the actual _button_. I have tried overriding the accent color of the three default themes (using SystemAccentColor and SystemControlHighlightAccentBrush), but only SystemAccentColor works and it affects all buttons _except_ the hamburger button.

[deltakosh]

Hello the best option would be to retemplate the control:
https://github.com/Microsoft/UWPCommunityToolkit/blob/dev/Microsoft.Toolkit.Uwp.UI.Controls/HamburgerMenu/HamburgerMenu.xaml

[imagin-tech]

Overriding template verbatim produces an unknown exception.

The error produced and offending line, respectively:

Problem goes away when you remove that one line.

[lukasf]

This is a new property that will be available in V1.4 of the toolkit. You need to remove it until the next update is available.

Nice example of how overriding control templates for common scenarios is bad. On next update you'd need to fixup your template copy in order to have full functionality of the control...

[deltakosh]

Can't agree more. And this is why I'm always for adding properties for common tasks