Skip to content

Contextual Menu: Moving Split Button to be its own component #4618

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
dzearing merged 21 commits into from
Apr 27, 2018
Merged

Contextual Menu: Moving Split Button to be its own component #4618

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
2301c62
moved split button out of the contextual menu
Apr 19, 2018
922d51d
Removed all references to touch
Apr 19, 2018
c7e22d3
added change files
Apr 19, 2018
e58ba9b
solved style and updated test
Apr 19, 2018
47e4cbd
fixed problem with submenu appearing in the wrong target button
Apr 19, 2018
4638505
fixed style issue
Apr 19, 2018
277d73d
removed unnecessary binds
Apr 20, 2018
254d3b7
reverted removing items in functions; set correct event target for an…
Apr 20, 2018
ba5c45a
addressed style problems
Apr 20, 2018
87aa983
updated snapshot
Apr 20, 2018
bc3ae34
updated bundle size
Apr 20, 2018
f0aa3d7
updated button styles with suggested changes
Apr 25, 2018
8e16927
separated the commands to be different autobind
Apr 27, 2018
ff88ae2
made target optional
Apr 27, 2018
c7a333a
renamed actions to be a different name
Apr 27, 2018
8fba90e
Merge branch 'master' into jspurlin/contextualMenuSplitButtonWithoutT…
dzearing Apr 27, 2018
364fa11
removed unused prop fields
Apr 27, 2018
dbf85ca
updated snapsthos
Apr 27, 2018
a55c76c
addressed comment to clean up code naming; bumped package size
Apr 27, 2018
ee4d5ec
resolved merge conflict
Apr 27, 2018
883e727
fixd lint error with closing tags
Apr 27, 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
47 changes: 47 additions & 0 deletions apps/vr-tests/src/stories/ContextualMenu.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,48 @@ const itemsWithHeaders = [
}
];

const itemsWithSplitButtonSubmenu = [
{
key: 'share',
split: true,
onClick: () => { },
subMenuProps: {
items: [
{
key: 'sharetotwitter',
name: 'Share to Twitter',
},
{
key: 'sharetofacebook',
name: 'Share to Facebook',
},
{
key: 'sharetoemail',
split: true,
onClick: () => { },
name: 'Share to Email',
subMenuProps: {
items: [
{
key: 'sharetooutlook_1',
name: 'Share to Outlook',
title: 'Share to Outlook',
},
{
key: 'sharetogmail_1',
name: 'Share to Gmail',
title: 'Share to Gmail',
}
],
},
},
],
},
name: 'Share'
}
];


storiesOf('ContextualMenu', module)
.addDecorator(FabricDecorator)
.addDecorator(story => (
Expand Down Expand Up @@ -214,4 +256,9 @@ storiesOf('ContextualMenu', module)
<ContextualMenu
items={ itemsWithHeaders }
/>
))
.add('With split button submenu', () => (
<ContextualMenu
items={ itemsWithSplitButtonSubmenu }
/>
));
11 changes: 11 additions & 0 deletions ...fice-ui-fabric-react/jspurlin-contextualMenuSplitButtonWithoutTouch_2018-04-19-17-33.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": "Contextual Menu: moved out the split button to be its own component, ContextualMenuSplitButton",
"type": "minor"
}
],
"packageName": "office-ui-fabric-react",
"email": "chiechan@microsoft.com"
}
179 changes: 51 additions & 128 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 @@ -6,8 +6,7 @@ import {
IMenuItemClassNames,
IContextualMenuClassNames,
getContextualMenuClassNames,
getItemClassNames,
getSplitButtonVerticalDividerClassNames
getItemClassNames
} from './ContextualMenu.classNames';
import {
BaseComponent,
Expand All @@ -27,14 +26,12 @@ import {
css,
shouldWrapFocus
} from '../../Utilities';
import { hasSubmenu, getIsChecked } from '../../utilities/contextualMenu/index';
import { hasSubmenu, getIsChecked, isItemDisabled } from '../../utilities/contextualMenu/index';
import { withResponsiveMode, ResponsiveMode } from '../../utilities/decorators/withResponsiveMode';
import { Callout } from '../../Callout';
import { IIconProps } from '../../Icon';
import {
VerticalDivider
} from '../../Divider';
import { ContextualMenuItem } from './ContextualMenuItem';
import { ContextualMenuSplitButton } from './ContextualMenuSplitButton';

export interface IContextualMenuState {
expandedMenuItemKey?: string;
Expand Down Expand Up @@ -94,8 +91,6 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
private readonly _navigationIdleDelay: number = 250 /* ms */;
private _scrollIdleTimeoutId: number | undefined;

private _splitButtonContainers: Map<string, HTMLDivElement>;

private _adjustedFocusZoneProps: IFocusZoneProps;

constructor(props: IContextualMenuProps) {
Expand All @@ -114,7 +109,6 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext

this._isFocusingPreviousElement = false;
this._isScrollIdle = true;
this._splitButtonContainers = new Map();
}

public dismiss = (ev?: any, dismissAll?: boolean) => {
Expand Down Expand Up @@ -363,7 +357,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
const getClassNames = item.getItemClassNames || getItemClassNames;
const itemClassNames = getClassNames(
this.props.theme!,
this._isItemDisabled(item),
isItemDisabled(item),
(this.state.expandedMenuItemKey === item.key),
!!getIsChecked(item),
!!item.href,
Expand Down Expand Up @@ -517,7 +511,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
aria-expanded={ itemHasSubmenu ? item.key === expandedMenuItemKey : undefined }
aria-posinset={ focusableElementIndex + 1 }
aria-setsize={ totalItemCount }
aria-disabled={ this._isItemDisabled(item) }
aria-disabled={ isItemDisabled(item) }
style={ item.style }
onClick={ this._onAnchorClick.bind(this, item) }
onMouseEnter={ this._onItemMouseEnter.bind(this, item) }
Expand Down Expand Up @@ -581,7 +575,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
'aria-checked': !!isChecked,
'aria-posinset': focusableElementIndex + 1,
'aria-setsize': totalItemCount,
'aria-disabled': this._isItemDisabled(item),
'aria-disabled': isItemDisabled(item),
role: item.role || defaultRole,
style: item.style
};
Expand Down Expand Up @@ -610,102 +604,30 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
totalItemCount: number,
hasCheckmarks?: boolean,
hasIcons?: boolean): JSX.Element {
const { contextualMenuItemAs } = this.props;

return (
<div
ref={ (el: HTMLDivElement) =>
this._splitButtonContainers.set(item.key, el)
}
role={ 'button' }
aria-labelledby={ item.ariaLabel }
className={ classNames.splitContainer }
aria-disabled={ this._isItemDisabled(item) }
aria-haspopup={ true }
aria-describedby={ item.ariaDescription }
aria-checked={ item.isChecked || item.checked }
aria-posinset={ focusableElementIndex + 1 }
aria-setsize={ totalItemCount }
onMouseEnter={ this._onItemMouseEnter.bind(this, { ...item, subMenuProps: null, items: null }) }
onMouseLeave={ this._onMouseItemLeave.bind(this, { ...item, subMenuProps: null, items: null }) }
onMouseMove={ this._onItemMouseMove.bind(this, { ...item, subMenuProps: null, items: null }) }
onKeyDown={ this._onSplitContainerItemKeyDown.bind(this, item) }
onClick={ this._executeItemClick.bind(this, item) }
tabIndex={ 0 }
data-is-focusable={ true }
>
{ this._renderSplitPrimaryButton(item, classNames, index, hasCheckmarks!, hasIcons!) }
{ this._renderSplitDivider(item) }
{ this._renderSplitIconButton(item, classNames, index) }
</div >
);
}

private _renderSplitPrimaryButton(item: IContextualMenuItem, classNames: IMenuItemClassNames, index: number, hasCheckmarks: boolean, hasIcons: boolean) {

const isChecked: boolean | null | undefined = getIsChecked(item);
const canCheck: boolean = isChecked !== null;
const defaultRole = canCheck ? 'menuitemcheckbox' : 'menuitem';
const { contextualMenuItemAs: ChildrenRenderer = ContextualMenuItem } = this.props;

const itemProps = {
key: item.key,
disabled: this._isItemDisabled(item) || item.primaryDisabled,
name: item.name,
className: classNames.splitPrimary,
role: item.role || defaultRole,
canCheck: item.canCheck,
isChecked: item.isChecked,
checked: item.checked,
icon: item.icon,
iconProps: item.iconProps,
'data-is-focusable': false,
'aria-hidden': true
} as IContextualMenuItem;
return React.createElement('button',
getNativeProps(itemProps, buttonProperties),
<ChildrenRenderer item={ itemProps } classNames={ classNames } index={ index } onCheckmarkClick={ hasCheckmarks ? this._onItemClick : undefined } hasIcons={ hasIcons } />,
);
}

private _onSplitContainerItemKeyDown(item: any, ev: React.KeyboardEvent<HTMLElement>) {
if (ev.which === KeyCodes.enter) {
this._executeItemClick(item, ev);
ev.preventDefault();
ev.stopPropagation();
} else {
this._onItemKeyDown(item, ev);
}
}

private _renderSplitIconButton(item: IContextualMenuItem, classNames: IMenuItemClassNames, index: number) {
const { contextualMenuItemAs: ChildrenRenderer = ContextualMenuItem } = this.props;
const itemProps = {
onClick: this._onSplitItemClick.bind(this, item),
disabled: this._isItemDisabled(item),
className: classNames.splitMenu,
subMenuProps: item.subMenuProps,
submenuIconProps: item.submenuIconProps,
split: true,
} as IContextualMenuItem;

return React.createElement('button',
assign({}, getNativeProps(itemProps, buttonProperties), {
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),
'data-is-focusable': false,
'aria-hidden': true
}),
<ChildrenRenderer item={ itemProps } classNames={ classNames } index={ index } hasIcons={ false } />
<ContextualMenuSplitButton
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 }
/>
);
}

private _renderSplitDivider(item: IContextualMenuItem) {
const getDividerClassnames = item.getSplitButtonVerticalDividerClassNames || getSplitButtonVerticalDividerClassNames;
return <VerticalDivider getClassNames={ getDividerClassnames } />;
}

private _getIconProps(item: IContextualMenuItem): IIconProps {
const iconProps: IIconProps = item.iconProps ? item.iconProps : {
iconName: item.icon
Expand Down Expand Up @@ -782,14 +704,22 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
}

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

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

@joschect joschect Apr 26, 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.

Maybe let target be optional since it's optional when going into focusMouseEvent. Not sure about this though, I'll leave it up to you

if (!this._isScrollIdle) {
return;
}

this._updateFocusOnMouseEvent(item, ev);
this._updateFocusOnMouseEvent(item, ev, target);
}

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

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

const targetElement = ev.currentTarget as HTMLElement;

Expand All @@ -799,9 +729,8 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
return;
}

this._updateFocusOnMouseEvent(item, ev);
this._updateFocusOnMouseEvent(item, ev, target);
}

private _onMouseItemLeave = (item: any, ev: React.MouseEvent<HTMLElement>): void => {
if (!this._isScrollIdle) {
return;
Expand Down Expand Up @@ -833,8 +762,8 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
* As part of updating focus, This function will also update
* the expand/collapse state accordingly.
*/
private _updateFocusOnMouseEvent(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>) {
const targetElement = ev.currentTarget as HTMLElement;
private _updateFocusOnMouseEvent(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>, target: HTMLElement) {
const targetElement = target;

@jspurlin jspurlin Apr 19, 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.

You should make target optional so that it can be leveraged only when it needs to be

const { subMenuHoverDelay: timeoutDuration = this._navigationIdleDelay } = this.props;

if (item.key === this.state.expandedMenuItemKey) {
Expand All @@ -857,9 +786,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
ev.stopPropagation();
this._enterTimerId = this._async.setTimeout(() => {
targetElement.focus();
const splitButtonContainer = this._splitButtonContainers.get(item.key);
this._onItemSubMenuExpand(item,
((item.split && splitButtonContainer) ? splitButtonContainer : targetElement) as HTMLElement);
this._onItemSubMenuExpand(item, targetElement);
this._enterTimerId = undefined;
}, this._navigationIdleDelay);
} else {
Expand All @@ -871,26 +798,25 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
}
}

private _onItemMouseDown(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>) {
if (item.onMouseDown) {
private _onItemMouseDown = (item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>): void => {
if (item.onMouseDown && this._enterTimerId === undefined) {

@jspurlin jspurlin Apr 19, 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.

why is this change here? mousedown should not be gated by the enterTimerId

@chang47 chang47 Apr 19, 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.

You're right, this was back when we were trying to fight the race condition caused by hovering onon the secondary button in split button vs clicking on it where we would open and close the menu.

This should have been removed.

item.onMouseDown(item, ev);
}
}

private _onItemClick(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>) {
private _onItemClick = (item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>): void => {
this._onItemClickBase(item, ev, ev.currentTarget as HTMLElement);
}

private _onSplitItemClick(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>) {
const splitButtonContainer = this._splitButtonContainers.get(item.key);
// get the whole splitButton container to base the menu off of
this._onItemClickBase(item, ev,
(splitButtonContainer ? splitButtonContainer : ev.currentTarget) as HTMLElement);
}

private _onItemClickBase(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement>, target: HTMLElement) {
private _onItemClickBase = (item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>, target: HTMLElement): void => {
const items = getSubmenuItems(item);

// Cancel a async menu item hover timeout action from being taken and instead
// just trigger the click event instead.
if (this._enterTimerId !== undefined) {
this._async.clearTimeout(this._enterTimerId);
this._enterTimerId = undefined;
}
if (!hasSubmenu(item) && (!items || !items.length)) { // This is an item without a menu. Click it.
this._executeItemClick(item, ev);
} else {
Expand All @@ -910,10 +836,11 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
ev.stopPropagation();
}

private _executeItemClick(item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>) {
private _executeItemClick = (item: IContextualMenuItem, ev: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>): void => {
if (item.disabled || item.isDisabled) {
return;
}

let dismiss = false;
if (item.onClick) {
dismiss = !!item.onClick(ev, item);
Expand All @@ -924,7 +851,7 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
(dismiss || !ev.defaultPrevented) && this.dismiss(ev, true);
}

private _onItemKeyDown(item: any, ev: React.KeyboardEvent<HTMLElement>) {
private _onItemKeyDown = (item: any, ev: React.KeyboardEvent<HTMLElement>): void => {
const openKey = getRTL() ? KeyCodes.left : KeyCodes.right;

if (ev.which === openKey && !item.disabled) {
Expand Down Expand Up @@ -1032,10 +959,6 @@ export class ContextualMenu extends BaseComponent<IContextualMenuProps, IContext
}
}

private _isItemDisabled(item: IContextualMenuItem): boolean {
return !!(item.isDisabled || item.disabled);
}

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

Expand Down
Loading