Changes to menuItemLocationChange #1285

IbraheemOsama · 2017-07-04T11:54:30Z

Original PR #1284
Fixing issue #1293
Fixing another issue #1301
Fixing issue #1352
Fixing issue #1267
and Adding underline character on pressing Alt

…oft/menuitemLocationFix

IbraheemOsama · 2017-07-14T08:45:54Z

@skendrot @nmetulev ping :)

IbraheemOsama · 2017-07-26T12:16:27Z

@nmetulev @skendrot ping ping

nmetulev · 2017-07-26T16:29:53Z

I'll try to review it this week

nmetulev

Auto FlyoutPlacement does not seem to work properly when changing orientation in run time. For example: in the sample app, starting with Horizontal orientation and then moving to Vertical, the flyouts show up on bottom instead on the right,

nmetulev · 2017-07-26T17:56:22Z

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

+    /// <summary>
+    /// Defines constants that specify the preferred location for positioning the menu Flyout
+    /// </summary>
+    public enum MenuFlyoutPlacement


The name of the file name doesn't match the name of the enum

Orientation and file name are fixed

skendrot · 2017-07-27T14:35:33Z

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


+        /// <summary>
+        /// Gets or sets the menu flyout placement
+        /// </summary>


I don't see why we need this new property and class? This should always be "auto". If the orientation is horizontal, then it's bottom or top (if the menu is at the bottom of the screen for some reason). If orientation is vertical it's either right or left

What if the menu is at the middle of the screen !! I added it so it should be more fixable for user to choose whatever he wants and with "Auto" option we are supporting the default option.

What if, what if, what if... Menus have a predefined behavior. That's what should be supported. Even allowing a vertical orientation is a stretch.

I actually agree with @skendrot on this one, It just seems like something that would be more trouble than worth. Should be all handled auto.

skendrot · 2017-07-27T14:35:54Z

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

        }

        private void MenuFlyout_Opened(object sender, object e)
        {


Much better than LayoutUpdated!!

Thanks for the idea

skendrot · 2017-07-27T14:37:10Z

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


        private bool AllowTooltip => (bool)GetValue(AllowTooltipProperty);

-        private static bool NavigateUsingKeyboard(object element, KeyEventArgs args, Menu menu, Orientation orientation)


Why move all of this into a different file?

It was easier to search where are events and where are helper methods. the file was too big I should have planned the logic well from the beginning but we added many features during the PR :)

IbraheemOsama · 2017-08-05T14:29:21Z

@nmetulev @skendrot I fixed issue #1352 #1267 #1266 and added underline to menuItem character if preceded by ^

nmetulev

Left few comments. In addition, when running the sample app with the menu in the bottom, the flyouts do not show up above the MenuItems as expected - instead they hide the MenuItems

Otherwise, love all the updated :)

nmetulev · 2017-08-06T19:30:04Z

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


-    <controls:Menu AllowTooltip="@[AllowTooltip:Bool:True]"
+    <controls:Menu
+                  AllowTooltip="@[AllowTooltip:Bool:False]"


Could the formatting be fixed up in the bind file, it's a bit difficult to read?

nmetulev · 2017-08-06T19:44:34Z

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

+
+            var runWithUnderlinedCharacter = new Run
+            {
+                Text = underlinedCharacter.ToString(),


TextDecorations is only available in CU, it will crash in AU or older. Maybe do a check before using?

Good catch :)

nmetulev · 2017-08-06T19:45:04Z

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

+            }
+
+            var underlineCharacterIndex = _originalHeader.IndexOf(UnderlineCharacter);
+


Will this work if ^ is at the end of the string?

It will be removed in method HeaderPropertyChanged at line 496 :) so there is no need to check here again.

nmetulev · 2017-08-06T19:51:48Z

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


+        /// <summary>
+        /// Gets or sets the menu flyout placement
+        /// </summary>


I actually agree with @skendrot on this one, It just seems like something that would be more trouble than worth. Should be all handled auto.

IbraheemOsama · 2017-08-07T00:06:51Z

@nmetulev regarding your comment 'when running the sample app with the menu in the bottom, the flyouts do not show up above the MenuItems as expected - instead they hide the MenuItems'
That's why I created FlyoutPlacement if you're at the bottom and you want the menu to show at the top you have to choose FlyoutPlacement and we just leave the placement to the developer.
We will provide the Auto for just the bottom and right Flyout. if you need top or left you'll have to use the FlyoutPlacement.
That's why I see FlyoutPlacement is useful and give a bit of space for the developer to choose :)

nmetulev · 2017-08-07T01:37:36Z

@IbraheemOsama, why not have Auto handle all the cases, is there a reason for that? Seems incomplete otherwise.

IbraheemOsama · 2017-08-12T07:21:58Z

@nmetulev @skendrot I removed FlyoutPlacement

nmetulev · 2017-08-16T05:58:10Z

Thanks @IbraheemOsama.

Sometimes the flyout shows up on the wrong side:

When menu is at the bottom, the flyout is aligned in the center, can it be left aligned?

Can you take a look?

IbraheemOsama · 2017-08-17T04:35:48Z

@nmetulev for the bottom menu, to align the menu to the left, we'll have to give it a custom position, I tried to apply the no space behavior to the bottom flyout but it behaved in a weird way.

regarding the wrong side issue can you provide more details so I can debug, because I calculate based on the distance between right and left sides, if right side is closer I show the menu to the left etc.

nmetulev · 2017-08-18T20:28:39Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Menu/MenuPage.xaml

        <StackPanel Grid.Row="1">
            <TextBlock Text="Click Alt to set focus on Classic Menu" />
-            <TextBlock Text="Any menu shortcut is Alt + (The first character of the menu name), ex to open File menu click Alt+F" />
+            <TextBlock Text="To know any menu shortcut click Alt + (The underlined character in case underline ^ is used), ex to open File menu click Alt+F" />


This line is unclear, could you update it to something along the lines of: Hold Alt to underline shortcut character. In code, ^ is used before the character you want highlighted"

nmetulev · 2017-08-18T20:33:15Z

I can no longer reproduce the placement issues so we should be good there.

On the bottom and right placement, you are absolutely correct, there is no easy way to manage that. We should look into using custom control for the flyout in the future if there is a need for this.

The only request is the text in the sample and we should be good to go (see comment above).

IbraheemOsama · 2017-08-19T00:59:24Z

@nmetulev you need to trust me more :P :) I changed the comment in the sample page

michael-hawker

Looks good. I don't think I have anything blocking, but if you could clean up a couple of the documentation based comments, that'd be great. Thanks!

michael-hawker · 2017-08-22T08:03:22Z

Microsoft.Toolkit.Uwp.SampleApp/SamplePages/Menu/MenuPage.xaml

            <controls:MenuItem x:Name="FileMenu"
-                               controls:Menu.InputGestureText="Alt+F">
+                               controls:Menu.InputGestureText="Alt+F"
+                               Header="^File">


Noticed the SymbolIcon Header example was removed, can we not support both with the new ^?

you can add any template to the header, maybe a single Image or a stack panel or even a grid how can I know which element containing the text without work around :)
If it is requested then we could include an icon property in the menuItem or provide a way to access the required text element.

Should be fixed using FindChild()

michael-hawker · 2017-08-22T08:06:01Z

docs/controls/Menu.md


 ### TooltipPlacement
-Gets or sets the tooltip PlacementMode on MenuItem. (Bottom,Right,Mouse,Left and Top)
+Gets or sets the tooltip PlacementMode on MenuItem. (Bottom,Right,Mouse,Left and Top).


Spaces between items in list?

michael-hawker · 2017-08-22T08:08:38Z

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

    </controls:Menu>

    <StackPanel Grid.Row="1">
      <TextBlock Text="Click Alt to set focus on Classic Menu" />


Can we update 'Click Alt' to be 'Press Alt'?

michael-hawker · 2017-08-22T08:09:25Z

docs/controls/Menu.md


+## MenuItem Properties
+### Header
+Gets or sets the header of the MenuItem. if you added '^' before any header character this character will be highlighted on clicking or holding Alt, this feature is used to visualize which character can be used beside Alt to open this MenuItem.


-> 'highlighted on pressing or holding Alt'

michael-hawker · 2017-08-22T08:34:18Z

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

        }

-        private void MenuItem_LayoutUpdated(object sender, object e)
+        private void MenuFlyout_Opening(object sender, object e)


Is this blank method to override a default behavior or something? If so, can you add a comment?

Nice catch :) I was testing something but it seems I removed the event registration but didn't remove the event it self

it seems that I didn't event remove the event registration :(

michael-hawker · 2017-08-22T08:36:33Z

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

+            }
+
+            menuItem._originalHeader = null;
+


Related to earlier comment: If the Header is an object, it'd be great if you could use FindChild<TextBlock>() to see if there's another text-based object you could grab from?

yeah that would help resolving the icon issue :)

we can included this in a next release as I'll not be available for creating new features until weekend (after code freeze).

michael-hawker · 2017-08-22T08:39:21Z

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

+        {
+            foreach (MenuItem item in Items)
+            {
+                item.Underline();


Maybe a future work item, support the MenuItem children as well?

We can cover it in another release :)

…raheemOsama/UWPCommunityToolkit into Microsoft/menuitemLocationFix

IbraheemOsama · 2017-08-22T12:30:02Z

Now we can merge :)

nmetulev and others added 6 commits July 3, 2017 16:56

added fix for menuflyout not position correctly on edges

83502e1

renamed new method

97ba6fb

removed dependancy on items for size checking in menuFlyout

2f0737f

removed unnecessary line

61f265e

Adding MenyFlyoutPlacement

4ef0a32

Adding aligment for menu

ee6da6e

dnfclas added the cla-already-signed label Jul 4, 2017

IbraheemOsama added 8 commits July 6, 2017 23:17

Changing GetMenuFlyoutPlacementMode

c3a679a

Adding stop watch

7f7f4aa

Fixing menu performance issue

9701292

Adding file headers

8ea5760

Removing update layout

f66ce28

updating methods

cdcf389

Adding auto flyout adaptian if not bottom or right

a6343ea

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

53355ff

…oft/menuitemLocationFix

IbraheemOsama mentioned this pull request Jul 8, 2017

added fix for menuflyout not positioned correctly on edges #1284

Closed

IbraheemOsama added controls 🎛️ improvements ✨ labels Jul 8, 2017

IbraheemOsama added this to the v2.0 milestone Jul 8, 2017

IbraheemOsama added 5 commits July 8, 2017 16:58

remove merge changes.

fef7a5f

Adding short cut to FlyoutMenuItems

823f77b

Updating bind file

afba2cb

Merging from dev and solving conflicts

63a03de

Solving merge conflicts

e0739d5

Update menu documentation

5b3ebb7

nmetulev requested changes Jul 26, 2017

View reviewed changes

skendrot reviewed Jul 27, 2017

View reviewed changes

IbraheemOsama added 2 commits August 6, 2017 00:00

Fixing persisting tooltip issue

df64eb8

fixing issue CommunityToolkit#1352

aecc052

IbraheemOsama added 2 commits August 6, 2017 13:11

Remove alt click handle

14654e9

Fixing Tooltip issues caused by recent changes

9968b5b

nmetulev requested changes Aug 6, 2017

View reviewed changes

IbraheemOsama added 4 commits August 12, 2017 09:35

Check TextDecoration in CU

4f11134

readding FlyoutPlacement

fca289a

Updating menu documentations to add the highlight

fbb2ea8

removing menu placement

b598033

nmetulev requested changes Aug 18, 2017

View reviewed changes

nmetulev added the reviewers wanted label Aug 18, 2017

IbraheemOsama added 2 commits August 19, 2017 10:24

Update menu sample page

7e871b0

fixing typo

83f0e08

nmetulev approved these changes Aug 19, 2017

View reviewed changes

Merge branch 'dev' into Microsoft/menuitemLocationFix

18f246e

michael-hawker approved these changes Aug 22, 2017

View reviewed changes

IbraheemOsama added 3 commits August 22, 2017 22:15

Updating documentations and removing unused events

3c7f4d5

Merge branch 'Microsoft/menuitemLocationFix' of https://github.com/Ib…

7117d92

…raheemOsama/UWPCommunityToolkit into Microsoft/menuitemLocationFix

Fixing merge issue

589219e

azchohfi merged commit 6daff81 into CommunityToolkit:dev Aug 22, 2017

nmetulev mentioned this pull request Aug 22, 2017

Menu keyboard shortcuts should show when holding Alt #1267

Closed


		private bool AllowTooltip => (bool)GetValue(AllowTooltipProperty);

		private static bool NavigateUsingKeyboard(object element, KeyEventArgs args, Menu menu, Orientation orientation)

		}

		var underlineCharacterIndex = _originalHeader.IndexOf(UnderlineCharacter);

Changes to menuItemLocationChange #1285

Changes to menuItemLocationChange #1285

Uh oh!

Conversation

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

Uh oh!

Uh oh!

IbraheemOsama commented Jul 14, 2017

Uh oh!

IbraheemOsama commented Jul 26, 2017

Uh oh!

nmetulev commented Jul 26, 2017

Uh oh!

nmetulev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

nmetulev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IbraheemOsama commented Aug 7, 2017

Uh oh!

nmetulev commented Aug 7, 2017

Uh oh!

IbraheemOsama commented Aug 12, 2017

Uh oh!

nmetulev commented Aug 16, 2017

Uh oh!

IbraheemOsama commented Aug 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nmetulev commented Aug 18, 2017

Uh oh!

IbraheemOsama commented Aug 19, 2017

Uh oh!

michael-hawker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

IbraheemOsama commented Jul 4, 2017 •

edited

Loading

IbraheemOsama commented Aug 5, 2017 •

edited

Loading