Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kelseyyoung merged 13 commits into from
May 7, 2018
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
aafc5d3
Add open and dismiss functions that can be referenced by a contextual…
kelseyyoung May 2, 2018
90aa6a6
Add tests for new item functions
kelseyyoung May 2, 2018
a844d06
Merge remote-tracking branch 'upstream/master' into keyou/contextual-…
kelseyyoung May 2, 2018
da3bef7
Add rush change file
kelseyyoung May 2, 2018
4fda842
Remove binds in submenu function
kelseyyoung May 2, 2018
cad5754
Merge with upstream master
kelseyyoung May 2, 2018
8d73bf7
Move contextual menu button and anchor into their own component
kelseyyoung May 4, 2018
be9da3d
Merge remote-tracking branch 'upstream/master' into keyou/contextual-…
kelseyyoung May 4, 2018
863b40b
Move the open/dismiss functions back to ContextualMenuItem
kelseyyoung May 4, 2018
6fa83bb
Merge remote-tracking branch 'upstream/master' into keyou/contextual-…
kelseyyoung May 4, 2018
8699f31
Merge remote-tracking branch 'upstream/master' into keyou/contextual-…
kelseyyoung May 7, 2018
e06449a
Make base class ContextualMenuItemWrapper
kelseyyoung May 7, 2018
782e08f
Remove dismiss bind and pass through undefined at ContextualMenuItem
kelseyyoung May 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...changes/office-ui-fabric-react/keyou-contextual-menu-item-functions_2018-05-02-18-14.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "Add ContextualMenuItem functions to open and close menus",
"type": "minor"
}
],
"packageName": "office-ui-fabric-react",
"email": "keyou@microsoft.com"
}
61 changes: 60 additions & 1 deletion packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ import * as React from 'react';
import { Promise } from 'es6-promise';
import * as ReactTestUtils from 'react-dom/test-utils';
import {
KeyCodes
KeyCodes,
createRef
} from '../../Utilities';
import { FocusZoneDirection } from '../../FocusZone';

import { ContextualMenu, canAnyMenuItemsCheck } from './ContextualMenu';
import { IContextualMenuItem, ContextualMenuItemType } from './ContextualMenu.types';
import { IContextualMenuItemWrapper } from './ContextualMenuItemWrapper';
import { LayerBase as Layer } from '../Layer/Layer.base';

describe('ContextualMenu', () => {
Expand Down Expand Up @@ -859,4 +861,61 @@ describe('ContextualMenu', () => {
expect(canAnyMenuItemsCheck(items)).toEqual(true);
});
});

describe('IContextualMenuRenderItem function tests', () => {
const contextualItem = createRef<IContextualMenuItemWrapper>();
let menuDismissed: boolean;
const onDismiss = (ev?: any, dismissAll?: boolean) => { menuDismissed = true; };

beforeEach(() => {
menuDismissed = false;
const menu: IContextualMenuItem[] = [
{
name: 'Test1',
key: 'Test1',
componentRef: contextualItem,
subMenuProps: {
items: [
{
name: 'Test2',
key: 'Test2',
className: 'SubMenuClass'
},
{
name: 'Test3',
key: 'Test3',
className: 'SubMenuClass'
}
],
}
}
];

ReactTestUtils.renderIntoDocument<ContextualMenu>(
<ContextualMenu
onDismiss={ onDismiss }
items={ menu }
/>
);
});

it('openSubMenu will open the item`s submenu if present', () => {
contextualItem.value!.openSubMenu();
expect(document.querySelector('.SubMenuClass')).not.toEqual(null);
});

it('dismissSubMenu will close the item`s submenu if present', () => {
// Open the submenu with a click
const menuItem = document.querySelector('button.ms-ContextualMenu-link') as HTMLButtonElement;
ReactTestUtils.Simulate.click(menuItem);
expect(document.querySelector('.SubMenuClass')).not.toEqual(null);
contextualItem.value!.dismissSubMenu();
expect(document.querySelector('.SubMenuClass')).toEqual(null);
});

it('dismissMenu will close the item`s menu', () => {
contextualItem.value!.dismissMenu();
expect(menuDismissed).toEqual(true);
});
});
});
197 changes: 61 additions & 136 deletions packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,6 @@ import {
import {
BaseComponent,
IPoint,
anchorProperties,
buttonProperties,
getNativeProps,
assign,
getId,
getRTL,
Expand All @@ -31,8 +28,7 @@ import { withResponsiveMode, ResponsiveMode } from '../../utilities/decorators/w
import { Callout } from '../../Callout';
import { IIconProps } from '../../Icon';
import { ContextualMenuItem } from './ContextualMenuItem';
import { KeytipData } from '../../KeytipData';
import { ContextualMenuSplitButton } from './ContextualMenuSplitButton';
import { ContextualMenuSplitButton, ContextualMenuButton, ContextualMenuAnchor } from './ContextualMenuItemWrapper';

export interface IContextualMenuState {
expandedMenuItemKey?: string;
Expand Down Expand Up @@ -485,58 +481,34 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
}

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);

@jspurlin jspurlin May 2, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kelseyyoung kelseyyoung May 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kelseyyoung kelseyyoung May 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@joschect joschect May 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@kelseyyoung kelseyyoung May 7, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@joschect joschect May 7, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const { expandedMenuItemKey } = this.state;
const { contextualMenuItemAs: ChildrenRenderer = ContextualMenuItem } = this.props;

let anchorRel = item.rel;
if (item.target && item.target.toLowerCase() === '_blank') {
anchorRel = anchorRel ? anchorRel : 'nofollow noopener noreferrer'; // Safe default to prevent tabjacking
}

const subMenuId = this._getSubMenuId(item);
const itemHasSubmenu = hasSubmenu(item);
const nativeProps = getNativeProps(item, anchorProperties);
const disabled = isItemDisabled(item);

return (
<div>
<KeytipData
keytipProps={ item.keytipProps }
ariaDescribedBy={ (nativeProps as any)['aria-describedby'] }
disabled={ disabled }
>
{ (keytipAttributes: any): JSX.Element => (
<a
{ ...nativeProps }
{ ...keytipAttributes }
href={ item.href }
target={ item.target }
rel={ anchorRel }
className={ classNames.root }
role='menuitem'
aria-owns={ item.key === expandedMenuItemKey ? subMenuId : undefined }
aria-haspopup={ itemHasSubmenu || undefined }
aria-expanded={ itemHasSubmenu ? item.key === expandedMenuItemKey : undefined }
aria-posinset={ focusableElementIndex + 1 }
aria-setsize={ totalItemCount }
aria-disabled={ isItemDisabled(item) }
style={ item.style }
onClick={ this._onAnchorClick.bind(this, item) }
onMouseEnter={ this._onItemMouseEnter.bind(this, item) }
onMouseLeave={ this._onMouseItemLeave.bind(this, item) }
onKeyDown={ itemHasSubmenu ? this._onItemKeyDown.bind(this, item) : null }
>
<ChildrenRenderer
item={ item }
classNames={ classNames }
index={ index }
onCheckmarkClick={ hasCheckmarks ? this._onItemClick : undefined }
hasIcons={ hasIcons }
/>
</a>
) }
</KeytipData>
</div>);
<ContextualMenuAnchor
item={ item }
classNames={ classNames }
index={ index }
focusableElementIndex={ focusableElementIndex }
totalItemCount={ totalItemCount }
hasCheckmarks={ hasCheckmarks }
hasIcons={ hasIcons }
contextualMenuItemAs={ contextualMenuItemAs }
onItemMouseEnter={ this._onItemMouseEnterBase }
onItemMouseLeave={ this._onMouseItemLeave }
onItemMouseMove={ this._onItemMouseMoveBase }
onItemMouseDown={ this._onItemMouseDown }
executeItemClick={ this._executeItemClick }
onItemClick={ this._onAnchorClick }
onItemKeyDown={ this._onItemKeyDown }
getSubMenuId={ this._getSubMenuId }
expandedMenuItemKey={ expandedMenuItemKey }
openSubMenu={ this._onItemSubMenuExpand }
dismissSubMenu={ this._onSubMenuDismiss }
dismissMenu={ dismissMenu }
componentRef={ item.componentRef }
/>
);
}

private _renderButtonItem(
Expand All @@ -547,79 +519,35 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
totalItemCount: number,
hasCheckmarks?: boolean,
hasIcons?: boolean) {
const { contextualMenuItemAs } = this.props;
const dismissMenu = this.dismiss.bind(this, undefined);
const { expandedMenuItemKey } = this.state;
const { contextualMenuItemAs: ChildrenRenderer = ContextualMenuItem } = this.props;

const subMenuId = this._getSubMenuId(item);
let ariaLabel = '';

if (item.ariaLabel) {
ariaLabel = item.ariaLabel;
} else if (item.name) {
ariaLabel = item.name;
}

const isChecked: boolean | null | undefined = getIsChecked(item);
const canCheck: boolean = isChecked !== null;
const defaultRole = canCheck ? 'menuitemcheckbox' : 'menuitem';
const itemHasSubmenu = hasSubmenu(item);

const buttonNativeProperties = getNativeProps(item, buttonProperties);
// Do not add the disabled attribute to the button so that it is focusable
delete (buttonNativeProperties as any).disabled;

const itemButtonProperties = {
className: classNames.root,
onClick: this._onItemClick.bind(this, item),
onKeyDown: itemHasSubmenu ? this._onItemKeyDown.bind(this, item) : null,
onMouseEnter: this._onItemMouseEnter.bind(this, item),
onMouseLeave: this._onMouseItemLeave.bind(this, item),
onMouseDown: (ev: any) => this._onItemMouseDown(item, ev),
onMouseMove: this._onItemMouseMove.bind(this, item),
href: item.href,
title: item.title,
'aria-label': ariaLabel,
'aria-haspopup': itemHasSubmenu || undefined,
'aria-owns': item.key === expandedMenuItemKey ? subMenuId : undefined,
'aria-expanded': itemHasSubmenu ? item.key === expandedMenuItemKey : undefined,
'aria-checked': !!isChecked,
'aria-posinset': focusableElementIndex + 1,
'aria-setsize': totalItemCount,
'aria-disabled': isItemDisabled(item),
role: item.role || defaultRole,
style: item.style
};

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

@kelseyyoung kelseyyoung May 4, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not properly copied over, will fix

@joschect joschect May 7, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you fixed this?

@kelseyyoung kelseyyoung May 7, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

keytipProps = {
...keytipProps,
hasMenu: true
};
}

return (
<KeytipData
keytipProps={ keytipProps }
ariaDescribedBy={ (buttonNativeProperties as any)['aria-describedby'] }
disabled={ isItemDisabled(item) }
>
{ (keytipAttributes: any): JSX.Element => (
<button
{ ...buttonNativeProperties as React.ButtonHTMLAttributes<HTMLButtonElement> }
{ ...itemButtonProperties as React.ButtonHTMLAttributes<HTMLButtonElement> }
{ ...keytipAttributes }
>
<ChildrenRenderer
item={ item }
classNames={ classNames }
index={ index }
onCheckmarkClick={ hasCheckmarks ? this._onItemClick : undefined }
hasIcons={ hasIcons }
/>
</button>
) }
</KeytipData>
<ContextualMenuButton
item={ item }
classNames={ classNames }
index={ index }
focusableElementIndex={ focusableElementIndex }
totalItemCount={ totalItemCount }
hasCheckmarks={ hasCheckmarks }
hasIcons={ hasIcons }
contextualMenuItemAs={ contextualMenuItemAs }
onItemMouseEnter={ this._onItemMouseEnterBase }
onItemMouseLeave={ this._onMouseItemLeave }
onItemMouseMove={ this._onItemMouseMoveBase }
onItemMouseDown={ this._onItemMouseDown }
executeItemClick={ this._executeItemClick }
onItemClick={ this._onItemClick }
onItemClickBase={ this._onItemClickBase }
onItemKeyDown={ this._onItemKeyDown }
getSubMenuId={ this._getSubMenuId }
expandedMenuItemKey={ expandedMenuItemKey }
openSubMenu={ this._onItemSubMenuExpand }
dismissSubMenu={ this._onSubMenuDismiss }
dismissMenu={ dismissMenu }
componentRef={ item.componentRef }
/>
);
}

Expand All @@ -632,6 +560,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
hasCheckmarks?: boolean,
hasIcons?: boolean): JSX.Element {
const { contextualMenuItemAs } = this.props;
const dismissMenu = this.dismiss.bind(this, undefined);

return (
<ContextualMenuSplitButton
Expand All @@ -651,6 +580,10 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
onItemClick={ this._onItemClick }
onItemClickBase={ this._onItemClickBase }
onItemKeyDown={ this._onItemKeyDown }
openSubMenu={ this._onItemSubMenuExpand }
dismissSubMenu={ this._onSubMenuDismiss }
dismissMenu={ dismissMenu }
componentRef={ item.componentRef }
onTap={ this._onPointerAndTouchEvent }
/>
);
Expand Down Expand Up @@ -731,10 +664,6 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
this._scrollIdleTimeoutId = this._async.setTimeout(() => { this._isScrollIdle = true; }, NavigationIdleDelay);
}

private _onItemMouseEnter = (item: any, ev: React.MouseEvent<HTMLElement>): void => {
this._onItemMouseEnterBase(item, ev, ev.currentTarget as HTMLElement);
}

private _onItemMouseEnterBase = (item: any, ev: React.MouseEvent<HTMLElement>, target?: HTMLElement): void => {
if (!this._isScrollIdle) {
return;
Expand All @@ -743,10 +672,6 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
this._updateFocusOnMouseEvent(item, ev, target);
}

private _onItemMouseMove(item: any, ev: React.MouseEvent<HTMLElement>) {
this._onItemMouseMoveBase(item, ev, ev.currentTarget as HTMLElement);
}

private _onItemMouseMoveBase = (item: any, ev: React.MouseEvent<HTMLElement>, target: HTMLElement): void => {

const targetElement = ev.currentTarget as HTMLElement;
Expand Down Expand Up @@ -857,7 +782,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
ev.preventDefault();
}

private _onAnchorClick(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>) {
private _onAnchorClick = (item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>) => {
this._executeItemClick(item, ev);
ev.stopPropagation();
}
Expand Down Expand Up @@ -895,7 +820,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
}
}

private _onItemSubMenuExpand(item: IContextualMenuItem, target: HTMLElement) {
private _onItemSubMenuExpand = (item: IContextualMenuItem, target: HTMLElement): void => {
if (this.state.expandedMenuItemKey !== item.key) {

if (this.state.expandedMenuItemKey) {
Expand Down Expand Up @@ -994,7 +919,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
}
}

private _getSubMenuId(item: IContextualMenuItem): string | undefined {
private _getSubMenuId = (item: IContextualMenuItem): string | undefined => {
let { subMenuId } = this.state;

if (item.subMenuProps && item.subMenuProps.id) {
Expand Down
7 changes: 6 additions & 1 deletion packages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenu.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { IContextualMenuClassNames, IMenuItemClassNames } from './ContextualMenu
export { DirectionalHint } from '../../common/DirectionalHint';
import { IVerticalDividerClassNames } from '../Divider/VerticalDivider.types';
import { IContextualMenuItemProps } from './ContextualMenuItem.types';
import { IContextualMenuItemWrapper } from './ContextualMenuItemWrapper';
import { IKeytipProps } from '../../Keytip';

export enum ContextualMenuItemType {
Expand Down Expand Up @@ -255,6 +256,11 @@ export interface IContextualMenuProps extends React.Props<ContextualMenu>, IWith
}

export interface IContextualMenuItem {
/**
* Optional callback to access the IContextualMenuItemWrapper interface. This will get passed down to the item that renders the ContextualMenuItem.
*/
componentRef?: (component: IContextualMenuItemWrapper | null) => void;

/**
* Unique id to identify the item
*/
Expand Down Expand Up @@ -471,7 +477,6 @@ export interface IContextualMenuItem {
* Optional prop to make an item readonly which is disabled but visitable by keyboard, will apply aria-readonly and some styling. Not supported by all components
*/
inactive?: boolean;

}

export interface IContextualMenuSection extends React.Props<ContextualMenu> {
Expand Down
Loading