Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
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": "[email protected]"
}
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 { IContextualMenuRenderItem } from './ContextualMenuItem.types';
import { LayerBase as Layer } from '../Layer/Layer.base';

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

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

beforeEach(() => {
menuDismissed = false;
const menu: IContextualMenuItem[] = [
{
name: 'Test1',
key: 'Test1',
renderItemRef: 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);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ import {
getFirstFocusable,
getLastFocusable,
css,
shouldWrapFocus
shouldWrapFocus,
createRef
} from '../../Utilities';
import { hasSubmenu, getIsChecked, isItemDisabled } from '../../utilities/contextualMenu/index';
import { withResponsiveMode, ResponsiveMode } from '../../utilities/decorators/withResponsiveMode';
Expand Down Expand Up @@ -499,6 +500,8 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
const itemHasSubmenu = hasSubmenu(item);
const nativeProps = getNativeProps(item, anchorProperties);
const disabled = isItemDisabled(item);
const anchorRef = createRef<HTMLAnchorElement>();
const dismissMenu = this.dismiss.bind(this, undefined);
Copy link
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

Copy link
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

Copy link
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


return (
<div>
Expand All @@ -511,6 +514,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
<a
{ ...nativeProps }
{ ...keytipAttributes }
ref={ anchorRef }
href={ item.href }
target={ item.target }
rel={ anchorRel }
Expand All @@ -534,6 +538,11 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
index={ index }
onCheckmarkClick={ hasCheckmarks ? this._onItemClick : undefined }
hasIcons={ hasIcons }
componentRef={ item.renderItemRef }
openSubMenu={ this._onItemSubMenuExpand }
dismissSubMenu={ this._onSubMenuDismiss }
dismissMenu={ dismissMenu }
subMenuTargetRef={ anchorRef }
/>
</a>
) }
Expand Down Expand Up @@ -599,6 +608,8 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
hasMenu: true
};
}
const btnRef = createRef<HTMLButtonElement>();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@jspurlin jspurlin May 3, 2018

Choose a reason for hiding this comment

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

@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

const dismissMenu = this.dismiss.bind(this, undefined);

return (
<KeytipData
Expand All @@ -608,6 +619,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
>
{ (keytipAttributes: any): JSX.Element => (
<button
ref={ btnRef }
{ ...buttonNativeProperties as React.ButtonHTMLAttributes<HTMLButtonElement> }
{ ...itemButtonProperties as React.ButtonHTMLAttributes<HTMLButtonElement> }
{ ...keytipAttributes }
Expand All @@ -618,6 +630,11 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
index={ index }
onCheckmarkClick={ hasCheckmarks ? this._onItemClick : undefined }
hasIcons={ hasIcons }
componentRef={ item.renderItemRef }
openSubMenu={ this._onItemSubMenuExpand }
dismissSubMenu={ this._onSubMenuDismiss }
dismissMenu={ dismissMenu }
subMenuTargetRef={ btnRef }
/>
</button>
) }
Expand All @@ -635,6 +652,10 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
hasIcons?: boolean): JSX.Element {
const { contextualMenuItemAs } = this.props;

const openSubMenu = this._onItemSubMenuExpand.bind(this);
const dismissSubMenu = this._onSubMenuDismiss.bind(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to use bind?

const dismissMenu = this.dismiss.bind(this, undefined);

return (
<ContextualMenuSplitButton
item={ item }
Expand All @@ -653,6 +674,10 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
onItemClick={ this._onItemClick }
onItemClickBase={ this._onItemClickBase }
onItemKeyDown={ this._onItemKeyDown }
openSubMenu={ openSubMenu }
dismissSubMenu={ dismissSubMenu }
dismissMenu={ dismissMenu }
componentRef={ item.renderItemRef }
/>
);
}
Expand Down Expand Up @@ -889,7 +914,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
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { IWithResponsiveModeState } from '../../utilities/decorators/withRespons
import { IContextualMenuClassNames, IMenuItemClassNames } from './ContextualMenu.classNames';
export { DirectionalHint } from '../../common/DirectionalHint';
import { IVerticalDividerClassNames } from '../Divider/VerticalDivider.types';
import { IContextualMenuItemProps } from './ContextualMenuItem.types';
import { IContextualMenuItemProps, IContextualMenuRenderItem } from './ContextualMenuItem.types';
import { IKeytipProps } from '../../Keytip';

export enum ContextualMenuItemType {
Expand Down Expand Up @@ -462,6 +462,11 @@ export interface IContextualMenuItem {
*/
keytipProps?: IKeytipProps;

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

/**
* Any additional properties to use when custom rendering menu items.
*/
Expand All @@ -471,7 +476,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
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';
import { hasSubmenu, getIsChecked } from '../../utilities/contextualMenu/index';
import { getRTL } from '../../Utilities';
import { BaseComponent, getRTL } from '../../Utilities';
import { Icon } from '../../Icon';
import { IContextualMenuItemProps } from './ContextualMenuItem.types';

Expand Down Expand Up @@ -67,19 +67,42 @@ const renderSubMenuIcon = ({ item, classNames }: IContextualMenuItemProps) => {
return null;
};

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Any decision here?

public render() {
const { item, classNames } = this.props;

return (
<div
className={
item.split ? classNames.linkContentMenu : classNames.linkContent
}
>
{ renderCheckMarkIcon(props) }
{ renderItemIcon(props) }
{ renderItemName(props) }
{ renderSubMenuIcon(props) }
</div>
);
};
return (
<div
className={
item.split ? classNames.linkContentMenu : classNames.linkContent
}
>
{ renderCheckMarkIcon(this.props) }
{ renderItemIcon(this.props) }
{ renderItemName(this.props) }
{ renderSubMenuIcon(this.props) }
</div>
);
}

public openSubMenu = (): void => {
const { item, openSubMenu, subMenuTargetRef } = this.props;
if (hasSubmenu(item) && openSubMenu && subMenuTargetRef && subMenuTargetRef.current) {
openSubMenu(item, subMenuTargetRef.current);
}
}

public dismissSubMenu = (): void => {
const { item, dismissSubMenu } = this.props;
if (hasSubmenu(item) && dismissSubMenu) {
dismissSubMenu();
}
}

public dismissMenu = (dismissAll?: boolean): void => {
const { dismissMenu } = this.props;
if (dismissMenu) {
dismissMenu(dismissAll);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,31 @@
import { IContextualMenuItem } from './ContextualMenu.types';
import { IMenuItemClassNames } from './ContextualMenu.classNames';
import { RefObject } from '../../Utilities';

export interface IContextualMenuItemProps
extends React.HTMLAttributes<IContextualMenuItemProps> {
export interface IContextualMenuRenderItem {
/**
* Function to open this item's subMenu, if present.
*/
openSubMenu: () => void;

/**
* Function to close this item's subMenu, if present.
*/
dismissSubMenu: () => void;

/**
* Dismiss the menu this item belongs to.
*/
dismissMenu: (dismissAll?: boolean) => void;
}

export interface IContextualMenuItemProps extends React.HTMLAttributes<IContextualMenuItemProps> {

/**
* Optional callback to access the IContextualMenuRenderItem interface. Use this instead of ref for accessing
* the public methods and properties of the component.
*/
componentRef?: (component: IContextualMenuRenderItem | null) => void;

/**
* The item to display
Expand All @@ -28,4 +51,25 @@ export interface IContextualMenuItemProps
* Click handler for the checkmark
*/
onCheckmarkClick?: ((item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>) => void);

/**
* This prop will get set by ContextualMenu and can be called to open this item's subMenu, if present.
*/
openSubMenu?: (item: IContextualMenuItem, target: HTMLElement) => void;

/**
* This prop will get set by ContextualMenu and can be called to close this item's subMenu, if present.
*/
dismissSubMenu?: () => void;

/**
* This prop will get set by ContextualMenu and can be called to close the menu this item belongs to.
* If dismissAll is true, all menus will be closed.
*/
dismissMenu?: (dismissAll?: boolean) => void;

/**
* This prop will get set by ContextualMenu, will be the target to attach the submenu to when openSubMenu is triggered.
*/
subMenuTargetRef?: RefObject<HTMLElement>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
} from './ContextualMenu.classNames';
import { ContextualMenuItem } from './ContextualMenuItem';
import { KeytipData } from '../../KeytipData';
import { getIsChecked, isItemDisabled } from '../../utilities/contextualMenu/index';
import { getIsChecked, isItemDisabled, hasSubmenu } from '../../utilities/contextualMenu/index';
import { VerticalDivider } from '../../Divider';
import { IContextualMenuSplitButtonProps } from './ContextualMenuSplitButton.types';

Expand Down Expand Up @@ -74,6 +74,27 @@ export class ContextualMenuSplitButton extends BaseComponent<IContextualMenuSpli
);
}

public openSubMenu = (): void => {
const { item, openSubMenu } = this.props;
if (hasSubmenu(item) && openSubMenu && this._splitButton) {
openSubMenu(item, this._splitButton);
}
}

public dismissSubMenu = (): void => {
const { item, dismissSubMenu } = this.props;
if (hasSubmenu(item) && dismissSubMenu) {
dismissSubMenu();
}
}

public dismissMenu = (dismissAll?: boolean): void => {
const { dismissMenu } = this.props;
if (dismissMenu) {
dismissMenu(dismissAll);
}
}

private _renderSplitPrimaryButton(item: IContextualMenuItem, classNames: IMenuItemClassNames, index: number, hasCheckmarks: boolean, hasIcons: boolean) {
const isChecked: boolean | null | undefined = getIsChecked(item);
const canCheck: boolean = isChecked !== null;
Expand Down
Loading