Add HamburgerVisibility property #1020

Merged
merged 1 commit into from Mar 13, 2017

Conversation

Projects
None yet
5 participants
@lukasf
Contributor

lukasf commented Mar 9, 2017

Added the HamburgerVisibility property as discussed in #1019.

@@ -54,6 +54,7 @@
DisplayMode="@[DisplayMode:Enum:SplitViewDisplayMode.CompactInline]"
CompactPaneLength="@[CompactPaneLength:Slider:48:10-80]"
HamburgerHeight="@[HamburgerHeight:Slider:48:10-80]"
+ HamburgerVisibility="@[HamburgerVisibility:Enum:Visibility.Visible]"

This comment has been minimized.

@deltakosh

deltakosh Mar 9, 2017

Contributor

I would call it HamburgerButtonVisibility to avoid misunderstandings

@deltakosh

deltakosh Mar 9, 2017

Contributor

I would call it HamburgerButtonVisibility to avoid misunderstandings

This comment has been minimized.

@lukasf

lukasf Mar 10, 2017

Contributor

@deltakosh That was my first idea as well. But then I saw that all other properties of the hamburger button only have the "Hamburger" prefix (HamburgerHeight, HamburgerWidth, HamburgerMargin, they all affect the button and not the icon itself). So for having naming consistency, I chose to use "HamburgerVisibility". But I agree that HamburgerButtonVisibility is the better name. Shall I change it?

@lukasf

lukasf Mar 10, 2017

Contributor

@deltakosh That was my first idea as well. But then I saw that all other properties of the hamburger button only have the "Hamburger" prefix (HamburgerHeight, HamburgerWidth, HamburgerMargin, they all affect the button and not the icon itself). So for having naming consistency, I chose to use "HamburgerVisibility". But I agree that HamburgerButtonVisibility is the better name. Shall I change it?

This comment has been minimized.

@deltakosh

deltakosh Mar 10, 2017

Contributor

You are completely right. We can keep like it then :)

@deltakosh

deltakosh Mar 10, 2017

Contributor

You are completely right. We can keep like it then :)

@Odonno

Odonno approved these changes Mar 11, 2017

@IbraheemOsama IbraheemOsama merged commit 0670343 into Microsoft:dev Mar 13, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment