Classic menu #1086

IbraheemOsama · 2017-04-09T03:40:42Z

Creating new control Classic Menu #981

…cMenu

ScottIsAFool · 2017-04-09T10:02:03Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenu.cs

+ /// </summary>
+ public static readonly DependencyProperty OrientationProperty =
+ DependencyProperty.Register(
+ "Orientation",


ScottIsAFool · 2017-04-09T10:03:48Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenuItem.cs

+ var menuItem = item as MenuFlyoutItemBase;
+ if (menuItem != null)
+ {
+ menuFlyout.Items?.Add(menuItem);


You created menuFlyout, how would Items be null? is the ?. needed?

No, not needed

ScottIsAFool · 2017-04-09T10:08:33Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenu.Events.cs

+
+ private void ClassicMenu_Loaded(object sender, RoutedEventArgs e)
+ {
+ var wrapPanel = (WrapPanel.WrapPanel)ItemsPanelRoot;


Should probably do ItemsPanelRoot as WrapPanel.WrapPanel given someone could change the ItemsPanel in the template. At which point, you should probably add checks for the main basic types of panel that contain an orientation property.

ScottIsAFool · 2017-04-09T10:09:28Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenu.Events.cs

+ var wrapPanel = (WrapPanel.WrapPanel)ItemsPanelRoot;
+ wrapPanel.Orientation = Orientation;
+
+ Dispatcher.AcceleratorKeyActivated -= Dispatcher_AcceleratorKeyActivated;


Other than here, when are these events every unsubscribed from? Should probably unsubscribe in the Unloaded event too.

ScottIsAFool · 2017-04-09T10:10:33Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenu.cs

+ static ClassicMenu()
+ {
+ MenuItemInputGestureCache =
+ new Dictionary<string, MenuFlyoutItem>();


Why is this on a new line? Better yet, why is this needed at all? Just instantiate the dictionary where it's declared.

ScottIsAFool · 2017-04-09T10:12:00Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenu.cs

+ /// <inheritdoc />
+ protected override void OnApplyTemplate()
+ {
+ Loaded -= ClassicMenu_Loaded;


Personal preference: I would move this line into the Loaded event handler.

Normally I would agree with this, but we run into issues if the user sets NavigationCacheMode. If we subscribe to the events in OnApplyTemplate and unsubscribe within the event handlers then we would not get the events the next time the page is hit.

ScottIsAFool · 2017-04-09T10:13:49Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenuItem.cs

+ }
+
+ /// <summary>
+ /// this method is used to show the menu for current item


Upper case T please.

ScottIsAFool · 2017-04-09T10:14:38Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ClassicMenu/ClassicMenu.bind

@@ -0,0 +1,123 @@
+<Page


Incorrect xaml in this file

ScottIsAFool · 2017-04-09T12:39:30Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenu.Events.cs

+
+ private void ClassicMenu_Unloaded(object sender, RoutedEventArgs e)
+ {
+ Dispatcher.AcceleratorKeyActivated -= Dispatcher_AcceleratorKeyActivated;


Just to be a stickler, unsubscribe from the Unloaded event in here too :)

I guess it should be done automatically ? Once the page is unloaded all events, properties should destroyed !!
Regarding the general events like Dispatcher.AcceleratorKeyActivated it make sense to unsubscribe at unloaded but does unsubscribe from unload event here helps ? :)

ScottIsAFool · 2017-04-09T15:15:17Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenu.Events.cs

+ /// </summary>
+ public partial class ClassicMenu
+ {
+ private static bool NavigateThrowMenuHeader(KeyEventArgs args, ClassicMenu menu, ClassicMenuItem menuItem, Orientation orientation)


I just realised what this method is actually for. It should be called NavigateThroughMenuHeader.

IbraheemOsama · 2017-04-10T10:23:34Z

@deltakosh I need an icon for the classic menu :)

skendrot · 2017-04-14T14:14:50Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenu.cs

+ private static readonly Dictionary<string, MenuFlyoutItem> MenuItemInputGestureCache = new Dictionary<string, MenuFlyoutItem>();
+
+ /// <summary>
+ /// Gets or sets the orientation of the Classic menu, Horizontal or vertical means that child controls will be added horizontally until the width of the panel can't fit more control then a new row is added to fit new horizontal added child controls, vertical means that child will be added vertically until the height of the panel is recieved then a new column is added


Add a few carriage returns please

not following what to do here ?

The comments in XML-docs are too long. You should split into multiple lines to make it more easy to read.

skendrot · 2017-04-14T14:15:35Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenu.cs

+ /// <summary>
+ /// Initializes a new instance of the <see cref="ClassicMenu"/> class.
+ /// </summary>
+ public ClassicMenu()


Can we put the ctor above the properties please. More of a personal preference than a must have

skendrot · 2017-04-14T14:35:01Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/samples.json

+ {
+ "Name": "ClassicMenu",
+ "Type": "ClassicMenuPage",
+ "About": "",


Need some information about this control

skendrot · 2017-04-14T14:39:46Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ClassicMenu/ClassicMenuPage.xaml

+
+ <Grid Background="{StaticResource Brush-Grey-05}">
+ <Grid.RowDefinitions>
+ <RowDefinition Height="1*" />


Height should be Auto. Looks odd having the huge space between the Menu and the page content. At first I thought it was bad sizing on the part of the menu itself

skendrot · 2017-04-14T14:40:22Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ClassicMenu/ClassicMenuPage.xaml

+ </MenuFlyoutSubItem>
+ </controls:ClassicMenuItem>
+
+ <controls:ClassicMenuItem Header="View" />


I kept clicking these items expecting a menu to open. Let's add items to these

skendrot · 2017-04-14T15:35:45Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenuItem.cs

+ /// <summary>
+ /// Gets or sets the menu style for ClassicMenuItem
+ /// </summary>
+ public Style MenuStyle


Why would each menu item have it's own menu style? wouldn't you want them all to be the same?

skendrot · 2017-04-14T15:36:06Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenuItem.cs

+ /// <summary>
+ /// Gets or sets the MenuBackground to appear in the title bar
+ /// </summary>
+ public string MenuBackground


Why would each menu item have it's own menu background? Wouldn't you want them all to be the same?

skendrot · 2017-04-14T15:37:44Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenuItem.cs

+ : FlyoutPlacementMode.Right;
+ menuFlyout.MenuFlyoutPresenterStyle = MenuStyle;
+
+ foreach (var item in Items)


Since this is an ItemsControl the ItemsSource could be bound to a backing collection and the items would not be menu flyouts. Should account for that now.

menuFlyout.Items.Add accepts only MenuFlyoutItemBase.
can you share how to limit the item source because I tried several approach with no luck

You could make it not an ItemsControl and have your own collection of Items

skendrot · 2017-04-14T15:38:22Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenuItem.cs

+ /// <summary>
+ /// Classic Menu Item is the items main container for Class Menu control
+ /// </summary>
+ public class ClassicMenuItem : ItemsControl


Why Classic MenuItem? Why not just MenuItem?

skendrot · 2017-04-14T15:39:09Z

Microsoft.Toolkit.Uwp.UI.Controls/ClassicMenu/ClassicMenu.cs

+ /// <summary>
+ /// Classic Menu Control defines a menu of choices for users to invoke.
+ /// </summary>
+ public partial class ClassicMenu : ItemsControl


Why Classic Menu? Why not just Menu?

skendrot · 2017-04-14T15:58:44Z

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

+ <Setter Property="Template">
+ <Setter.Value>
+ <ControlTemplate TargetType="local:ClassicMenuItem">
+ <Button x:Name="FlyoutButton"


Instead of keeping track of the Button, add the MenuFlyout within the xaml and access that within OnApplyTemplate

I tried but it didn't work so I used the button instead.

IbraheemOsama · 2017-06-25T13:34:04Z

I will push new changes, please don't review as final review

nmetulev · 2017-06-25T18:55:36Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Menu/Menu.bind

+ <Page.Resources>
+ <ResourceDictionary>
+ <ResourceDictionary.MergedDictionaries>
+ <ResourceDictionary Source="MenuStyles.xaml" />


I think this can be safely removed

nmetulev · 2017-06-25T18:56:10Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Menu/Menu.bind

+
+ </Page.Resources>
+
+ <Grid Background="{StaticResource Brush-Grey-05}">


Should we remove custom brushes from the bind file

nmetulev · 2017-06-25T18:57:04Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Menu/Menu.bind

+ <RowDefinition Height="auto" />
+ </Grid.RowDefinitions>
+
+ <controls:Menu RequestedTheme="Light"


Any reason why you are specifically requesting Light theme?

It was for testing purposes no need for it.

…cMenu

IbraheemOsama · 2017-06-26T03:51:41Z

@nmetulev @skendrot I won't say "now the PR contains everything", last time I said so I received more comments :D

…outItems

skendrot · 2017-06-27T14:09:59Z

Microsoft.Toolkit.Uwp.UI.Controls/Menu/Menu.cs

+
+ /// <summary>
+ /// Gets or sets the orientation of the Menu, Horizontal or vertical means that child controls will be added horizontally
+ /// until the width of the panel can't fit more control then a new row is added to fit new horizontal added child controls, 


Remove Trailing whitespace to fix warning

Approved but should still get all the warnings removed.

skendrot · 2017-06-27T14:13:26Z

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

+
+ <Style TargetType="local:MenuItem">
+ <Setter Property="IsTabStop" Value="True" />
+ <Setter Property="Background" Value="{ThemeResource SystemControlBackgroundBaseLowBrush}" />


I think the menu items should have a transparent background by default. This is what users would expect from a Menu

skendrot · 2017-06-27T14:15:05Z

Microsoft.Toolkit.Uwp.UI.Controls/Menu/Menu.cs

+ /// <summary>
+ /// Gets the current selected menu header item
+ /// </summary>
+ public MenuItem SelectedHeaderItem { get; internal set; }


Should this be SelectedMenuItem?

skendrot · 2017-06-27T16:55:35Z

Microsoft.Toolkit.Uwp.UI.Controls/Menu/Menu.cs

+
+ /// <summary>
+ /// Gets or sets the orientation of the Menu, Horizontal or vertical means that child controls will be added horizontally
+ /// until the width of the panel can't fit more control then a new row is added to fit new horizontal added child controls, 


Approved but should still get all the warnings removed.

…cMenu

IbraheemOsama added 6 commits April 9, 2017 11:21

CommunityToolkit#981 adding ClassicMenu Control

856ccb9

CommunityToolkit#981 creating parital classes

2388c94

CommunityToolkit#981 fixing double navigation issue

b1d7520

CommunityToolkit#981 adding clarifications to the sample page.

b8fd598

CommunityToolkit#981 removing unused references

0f0dd32

CommunityToolkit#981 adding data to edit menu

61acc21

dnfclas added the cla-already-signed label Apr 9, 2017

IbraheemOsama requested a review from deltakosh April 9, 2017 03:40

IbraheemOsama added 4 commits April 9, 2017 19:15

CommunityToolkit#981 fixing layout and adding comments

aea3bb5

CommunityToolkit#981 adding headers to all files

43e96a1

Merge remote-tracking branch 'refs/remotes/Microsoft/dev' into Classi…

1d43869

…cMenu

CommunityToolkit#981 adding header to VsCommands file

091faaf

IbraheemOsama added the controls 🎛️ label Apr 9, 2017

IbraheemOsama added this to the v1.5 milestone Apr 9, 2017

CommunityToolkit#981 Apply Xaml styler roles to new Xaml files

ecd0544

ScottIsAFool reviewed Apr 9, 2017

View reviewed changes

IbraheemOsama added 2 commits April 9, 2017 21:39

Adding colors to Classic Menu

201644f

Applying some PR comments

11e95c0

ScottIsAFool reviewed Apr 9, 2017

View reviewed changes

IbraheemOsama added 3 commits April 10, 2017 17:15

Adding styling

39a7b2b

Adding items to edit menu

0e6637d

Creating a circular menu items

d6be89d

Enhancing circular menu performance

4cc9d0e

skendrot suggested changes Apr 14, 2017

View reviewed changes

IbraheemOsama added 3 commits April 18, 2017 20:52

changing classic menu to menu

3693abf

adding border to items presenter

dcfef83

generlization the menu style

a20c4de

IbraheemOsama added 3 commits June 25, 2017 10:44

prioritiese menu flyout popup than AccessKey

184731f

fixing caching problem

da18bf4

fixing IsInTransitionState

b529aa9

Adding removing input gesture on menunloaded

eafdcde

nmetulev requested changes Jun 25, 2017

View reviewed changes

IbraheemOsama added 2 commits June 26, 2017 13:43

Removing cached data on menu unload event

fa79558

Merge remote-tracking branch 'refs/remotes/Microsoft/dev' into Classi…

f2877f0

…cMenu

nmetulev approved these changes Jun 27, 2017

View reviewed changes

IbraheemOsama added 6 commits June 27, 2017 19:17

Merging for dev

41ea514

fixing merge conflicts

6fde02e

Addine menu to readme.md

d931857

Adding documentations

03c7502

Updating documentations

50ba981

Removing AccessKey from documentation as it is not working in MenuFly…

4a143ae

…outItems

skendrot suggested changes Jun 27, 2017

View reviewed changes

skendrot reviewed Jun 27, 2017

View reviewed changes

skendrot approved these changes Jun 27, 2017

View reviewed changes

skendrot suggested changes Jun 27, 2017

View reviewed changes

IbraheemOsama added 5 commits June 28, 2017 07:04

Merge remote-tracking branch 'refs/remotes/Microsoft/dev' into Classi…

57a3d67

…cMenu

Removing warning

ab21155

Renaming SelectedHeaderItem to SelectedMenuItem

9324573

Removing menu background

5e19294

Merge remote-tracking branch 'refs/remotes/Microsoft/dev' into Classi…

9fee708

…cMenu

skendrot approved these changes Jun 27, 2017

View reviewed changes

skendrot merged commit 8a54ae5 into CommunityToolkit:dev Jun 27, 2017

This was referenced Jun 27, 2017

Create a "classic menu" control #981

Closed

Menu Control: When expanded, opening other menus on mouse over #1257

Closed


		</Page.Resources>

		<Grid Background="{StaticResource Brush-Grey-05}">

Classic menu #1086

Classic menu #1086

Uh oh!

Conversation

IbraheemOsama commented Apr 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

IbraheemOsama commented Apr 10, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IbraheemOsama commented Jun 25, 2017

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

IbraheemOsama commented Jun 26, 2017

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

Labels

8 participants

IbraheemOsama commented Apr 9, 2017 •

edited

Loading