Add functions to ContextualMenuItem to open and close menus on command #4741

kelseyyoung · 2018-05-02T18:29:37Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

This work is related to keytips. It gives a way for a developer, given a ref to a contextual menu item, to open and close its menus on command instead of mocking a click or keypress. They are similar to the functions added to BaseButton (openMenu and dismissMenu). The functions added are:

openSubMenu() - opens an item's subMenu, if it has one
dismissSubMenu() - closes an item's subMenu, if it has one
dismissMenu(dismissAll?: boolean) - closes the menu this ContextualMenuItem lives in, or all menus if dismissAll is true. The dismissAll parameter is dependent on the developer's implementation of onDismiss (which is how it's always been)

For ContextualMenuItem, these functions will call optional props that are assigned by ContextualMenu. The openSubMenu prop function requires a target to attach the subMenu to; this is also assigned in ContextualMenu by passing down a ref to the container element (the anchor in _renderAnchorMenuItem and the button in _renderButtonItem)

ContextualMenuSplitButton also uses this interface and has access to these functions. It does not need to take in a ref for openSubMenu because the containing div (this._splitButton) already has a ref and is what the subMenu should attach to

Part of the change requires ContextualMenuItem to become a BaseComponent to take advantage of componentRef

Focus areas to test

Wrote unit tests and tested manually

… menu item

…menu-item-functions

jspurlin · 2018-05-02T19:34:43Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

+    const anchorRef = createRef<HTMLAnchorElement>();
+    const openSubMenu = this._onItemSubMenuExpand.bind(this);
+    const dismissSubMenu = this._onSubMenuDismiss.bind(this);
+    const dismissMenu = this.dismiss.bind(this, undefined);


I don't think you need to bind these sine the functions are defined a variables the this should be correct. And for dismissMenu you shound't need to pass in undefined since the parameters are optional

I'm passing undefined because this.dismiss takes in the event (optional) as the first param but dismissMenu takes in one param which is dismissAll, which is the second param in this.dismiss. So I'm padding the params basically

I will look into not binding them. I was just following the pattern throughout the file where there are binds

kelseyyoung · 2018-05-02T19:59:17Z

Hmmm I will see why I'm adding 350B...I wouldn't think it would be that much based on the changes

- Move private ContextualMenuItem functions back to local functions - Renames and refactoring

joschect

I'm tempted to say that we should approach this from another angle. Ref's should be avoided in general, and in this case props could be a way of expressing which item/menu should be open. I'm not sure how much more work is required but I do believe that that's the correct way to move in. @dzearing or @cliffkoh might disagree with me though.

joschect · 2018-05-02T21:39:25Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

        hasMenu: true
      };
    }
+    const btnRef = createRef<HTMLButtonElement>();


We really should avoid using refs whenever possible. Especially not in the way that this one is used. Instead of passing a ref down, pass a queryselector through or something like that. If anything the item itself should decide what the target is.

The ref is required so that the submenu positions itself correctly on the containing and elements (which is what they normally position on) which aren't available in ContextualMenuItem. An alternative approach is to move out the button and anchor items into their own component, like ContextualMenuSplitButton was moved. This would remove the need to pass down a ref (in the code you can see that I don't pass down that ref to ContextualMenuSplitButton). Again curious to hear @dzearing and @cliffkoh opinions

Also up for discussion: passing the componentRef to ContextualMenuItem through IContextualMenuItem and 'renderItemRef' in order to set the componentRef on ContextualMenuItem. Again pinging @dzearing and @cliffkoh

@kelseyyoung the best sounding approach you've described would be to split button and anchor into their own components ( ContextualMenuButton and ContextualMenuAnchor) to remove the need to pass down a ref

jspurlin · 2018-05-03T20:33:54Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

    const { contextualMenuItemAs } = this.props;

+    const openSubMenu = this._onItemSubMenuExpand.bind(this);
+    const dismissSubMenu = this._onSubMenuDismiss.bind(this);


Do these need to use bind?

- Move all wrapper items into their own folder under ContextualMenu

…menu-item-functions

kelseyyoung · 2018-05-04T00:36:55Z

@jspurlin and @joschect, I've updated the PR where Button and Anchor are moved to their own components, I've called these along with Split Button "ContextualMenuItemWrapper" components. This change means no real changes to ContextualMenuItem. The componentRef in IContextualMenuItem is now passed to one of these wrapper components

kelseyyoung · 2018-05-04T01:05:25Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx

-    };
-
-    let { keytipProps } = item;
-    if (keytipProps && itemHasSubmenu) {


This was not properly copied over, will fix

Have you fixed this?

kelseyyoung · 2018-05-04T01:25:43Z

After testing in WAC I'm having some problems where the internal ref that determines where to open the submenu isn't being set fast enough. I'll try to figure out why tomorrow

- Rename the prop to componentRef on IContextualMenuItem - Add tests for split button and anchor - Pass a prop function which returns the element to use in openSubMenu to ContextualMenuItem

…menu-item-functions

kelseyyoung · 2018-05-04T20:02:34Z

Another update: the previous iteration had issues where our internal refs weren't being correctly set. I've moved the open/dismiss functions back to ContextualMenuItem. For openSubMenu to get the element to position on, I've added a 'getContainerElement' prop in ContextualMenuItem which is set by the wrapper components (SplitButton, Button, Anchor). This involves refs internally in the wrapper components but doesn't involve passing them down to ContextualMenuItem. Would love to hear feedback from @joschect and @dzearing

jspurlin · 2018-05-04T20:54:11Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenuItem.tsx


-export const ContextualMenuItem: React.StatelessComponent<IContextualMenuItemProps> = (props) => {
-  const { item, classNames } = props;
+export class ContextualMenuItem extends BaseComponent<IContextualMenuItemProps, {}> {


Why did the extends change from React.StatelessComponent to BaseComponent?

To take advantage of componentRef. You can't use a ref on a functional component

Another option would be to just extend PureComponent and assign ref instead of componentRef. But then naming the prop 'componentRef' is a little misleading

Any decision here?

jspurlin · 2018-05-04T21:09:46Z

...abric-react/src/components/ContextualMenu/ContextualMenuItemWrapper/ContextualMenuAnchor.tsx

+          disabled={ disabled }
+        >
+          { (keytipAttributes: any): JSX.Element => (
+            <a


ContextualMenuAnchor and ContextualMenuButton are very similar (a different tag is used and a few different props are used). Did you consider creating a generic component that takes a tag and itemprops and would return the correct type of item?

This isn't a bad idea, you could even have the tag be a prop that get's passed down.

Jeremy and I talked about it; in my opinion there are still a few differences between the two and may require more work. I don't know if this PR should be that far reaching. I understand there's now some duplication across these two files, but I guess I would rather log a bug to fix this later

Can you reply with the bug info for the future work?

joschect · 2018-05-04T22:18:18Z

...abric-react/src/components/ContextualMenu/ContextualMenuItemWrapper/ContextualMenuAnchor.tsx

+                openSubMenu={ openSubMenu }
+                dismissSubMenu={ dismissSubMenu }
+                dismissMenu={ dismissMenu }
+                getContainerElement={ this._getContainerElement }


I like this solution much better overall. I might call it "getMenuTarget" or somehting like that

will rename to 'getSubmenuTarget'

joschect · 2018-05-06T20:35:57Z

packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx


  private _renderAnchorMenuItem(item: IContextualMenuItem, classNames: IMenuItemClassNames, index: number, focusableElementIndex: number, totalItemCount: number, hasCheckmarks: boolean, hasIcons: boolean): React.ReactNode {
+    const { contextualMenuItemAs } = this.props;
+    const dismissMenu = this.dismiss.bind(this, undefined);


Don't bind this here, you don't need to.

I think I have to here. dismiss takes in the event as the first argument but I only pass through the boolean dismissAll in my callback. So I'm binding undefined to only pass through the second argument, basically

The event can be undefined, it's fine to have it be undefined all the way up

joschect · 2018-05-06T20:44:09Z

...abric-react/src/components/ContextualMenu/ContextualMenuItemWrapper/ContextualMenuAnchor.tsx

+      </div>);
+  }
+
+  private _onItemMouseEnter = (ev: React.MouseEvent<HTMLElement>): void => {


Since these are the same as the ButtonItem methods, they should either get passed in as props, or inherited from some base component to reduce code duplication.

…menu-item-functions

- Move shared functions into protected members - Rename getContainerElement to getSubmenuTarget

kelseyyoung · 2018-05-07T18:06:12Z

Updated the PR, these are the changes:
Made a ContextualMenuItemWrapper base class. All three wrapper classes inherit from it. The shared functions are now protected in the base class. SplitButton still has a lot of private functions because of the large difference in it's functionality

* master: (52 commits) Marqueeselection style update (microsoft#4803) Applying package updates. FocusZone: Add the ability to stop focus from propagating outside the FocusZone (microsoft#4823) Unknown persona coin (microsoft#4809) Applying package updates. BaseButton: Allow Alt + Down on menu buttons to open the menu (microsoft#4807) Applying package updates. Website: Scroll to top of page on navigation (microsoft#4810) Applying package updates. ActivityItem: fix getstyles (microsoft#4802) Mark Slider#ValuePosition enum as deprecated as it is unused. (microsoft#4799) Icon: ensure `styles` still works. (microsoft#4805) Popup: Added check for onBlur to prevent focus setting focus to be incorrectly disabled when closing a menu in Chrome (microsoft#4804) Update package.json Move <Layer> to use React portals when available (microsoft#4724) Fix breadcrumb rendering issue when overflow item is at last index (microsoft#4787) Fixes focus issue for contextual menus (microsoft#4744) Remove redundant selected items prop on BaseExtendedPicker (microsoft#4769) SearchBox: New prop for turning off icon animation (microsoft#4794) Add functions to ContextualMenuItem to open and close menus on command (microsoft#4741) ...

kelseyyoung added 4 commits May 1, 2018 18:03

Add open and dismiss functions that can be referenced by a contextual…

aafc5d3

… menu item

Add tests for new item functions

90aa6a6

Merge remote-tracking branch 'upstream/master' into keyou/contextual-…

a844d06

…menu-item-functions

Add rush change file

da3bef7

kelseyyoung requested a review from joschect as a code owner May 2, 2018 18:29

jspurlin reviewed May 2, 2018

View reviewed changes

Remove binds in submenu function

4fda842

- Move private ContextualMenuItem functions back to local functions - Renames and refactoring

joschect reviewed May 2, 2018

View reviewed changes

Merge with upstream master

cad5754

jspurlin reviewed May 3, 2018

View reviewed changes

kelseyyoung added 2 commits May 3, 2018 17:27

Move contextual menu button and anchor into their own component

8d73bf7

- Move all wrapper items into their own folder under ContextualMenu

Merge remote-tracking branch 'upstream/master' into keyou/contextual-…

be9da3d

…menu-item-functions

kelseyyoung commented May 4, 2018

View reviewed changes

kelseyyoung added 2 commits May 4, 2018 12:54

Move the open/dismiss functions back to ContextualMenuItem

863b40b

- Rename the prop to componentRef on IContextualMenuItem - Add tests for split button and anchor - Pass a prop function which returns the element to use in openSubMenu to ContextualMenuItem

Merge remote-tracking branch 'upstream/master' into keyou/contextual-…

6fa83bb

…menu-item-functions

jspurlin reviewed May 4, 2018

View reviewed changes

joschect suggested changes May 6, 2018

View reviewed changes

kelseyyoung added 2 commits May 7, 2018 10:11

Merge remote-tracking branch 'upstream/master' into keyou/contextual-…

8699f31

…menu-item-functions

Make base class ContextualMenuItemWrapper

e06449a

- Move shared functions into protected members - Rename getContainerElement to getSubmenuTarget

joschect approved these changes May 7, 2018

View reviewed changes

Remove dismiss bind and pass through undefined at ContextualMenuItem

782e08f

kelseyyoung merged commit daa7b69 into microsoft:master May 7, 2018

kelseyyoung mentioned this pull request May 7, 2018

ContextualMenuButton and ContextualMenuAnchor should be combined into one file #4800

Closed

This was referenced May 25, 2018

Onclick event not invoked for contextual menu item with href #5002

Closed

Fix onitem click for contextual menu with anchor item #5003

Merged

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Add functions to ContextualMenuItem to open and close menus on command #4741

Add functions to ContextualMenuItem to open and close menus on command #4741

Uh oh!

Conversation

kelseyyoung commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Description of changes

Focus areas to test

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!

kelseyyoung commented May 2, 2018

Uh oh!

joschect 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!

jspurlin May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelseyyoung commented May 4, 2018

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!

kelseyyoung commented May 4, 2018

Uh oh!

kelseyyoung commented May 4, 2018

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!

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!

kelseyyoung commented May 7, 2018

Uh oh!

Reviewers

Assignees

Labels

kelseyyoung commented May 2, 2018 •

edited

Loading

jspurlin May 3, 2018 •

edited

Loading