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

chang47 · 2018-04-19T17:41:18Z

Problem:
The Contextual Menu is getting bloated and there is a pending change (#4353) that requires better state control for split buttons so the best fix to rectify this problem is to make the split button in the contextual menu its own component.

Solution:
I moved the code that rendered a contextual menu into a separate component and added all the previous event listeners as callbacks that will be called by the new component.

Validation:

The split button in the contextual menu still acts as it did before with mouse and keyboard interactions. That is the case with both checkmarks and normal items
The other buttons: anchor buttons, normal buttons, also remained unchanged, which is correct as the this change didn't touch those buttons.

chang47 · 2018-04-19T17:41:26Z

@jspurlin

jspurlin · 2018-04-19T19:46:13Z

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

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


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

jspurlin · 2018-04-19T19:53:08Z

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

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


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

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.

joschect · 2018-04-19T20:50:23Z

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

+export class ContextualMenuSplitButton extends BaseComponent<IContextualMenuSplitButtonProps, IContextualMenuSplitButtonState> {
+  private _processingTouch: boolean;
+  private _lastTouchTimeoutId: number | undefined;
+  private _splitButton: HTMLDivElement;


I don't see these used anywhere.

joschect · 2018-04-19T20:52:43Z

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

+        aria-checked={ item.isChecked || item.checked }
+        aria-posinset={ focusableElementIndex + 1 }
+        aria-setsize={ totalItemCount }
+        onMouseEnter={ this._onItemMouseEnter.bind(this, { ...item, subMenuProps: null, items: null }) }


since item is in props, move this logic to the private function, so for example, onItemMouseEnter would look like:

private _onItemMouseEnter = (ev: React.MouseEvent<HTMLElement>): void => { const { onItemMouseEnter, item } = this.props; if (onItemMouseEnter) { onItemMouseEnter({...item, submenuProps: null, items: null}, ev, this._splitButton); } }

Also since the function is defined as a lambda you don't need to bind it.

I made the changes, but after playing around with it, I decided that I'm going to keep it as is.

The way the split button before previously was used, we passed in a a item with no submenuProps or list of items when we enter the container, if we get rid of the binding like this, we'll have to make 2 functions, one that does the existing function and the other one that does the same thing, but passes in a modified item, otherwise we face runtime undefined code.

joschect · 2018-04-19T20:53:53Z

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

+    } as IContextualMenuItem;
+    return React.createElement('button',
+      getNativeProps(itemProps, buttonProperties),
+      <ChildrenRenderer data-is-focusable={ false } item={ itemProps } classNames={ classNames } index={ index } onCheckmarkClick={ hasCheckmarks && onItemClick ? onItemClick.bind(this, item) : undefined } hasIcons={ hasIcons } />,


Break this up into lines. Also lets go ahead and return instead of using create element. We really should phase createElement out entirely.

…chor button menu focus

christiango · 2018-04-23T23:47:08Z

How much of this is just a refactor vs an actual code change?

chang47 · 2018-04-24T00:01:31Z

@christiango Most of this is refactor, for some context if you aren't aware, this was made for my touch/tap code, but I decided that it was better to just make the change separate, so this is only the split button refactoring.

The 2 actual code change I did was:

Remove any mention of splitButtonContainer in Contextual Menu since the ContextualMenuSplitButton has the reference now and does its own state management
When I added onItemMouseEnterBase and _onItemMouseMoveBase. When we select the secondary button in the button, we would only give a reference to that button and not the the whole container, as a result, the submenu will show up only next to the secondary button, which is wrong if the submenu were to appear to the left of the button.

Regarding the second issue, what's interesting is that this was working prior to my change, so I'm going to look a bit at the problem. I think the reason I encountered it previously was because I was passing in the wrong target, but let me validate that again.

dzearing · 2018-04-25T04:32:46Z

...ages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenuSplitButton.types.ts

+import { IContextualMenuItemProps } from './ContextualMenuItem.types';
+import { ContextualMenuSplitButton } from './ContextualMenuSplitButton';
+
+export interface IContextualMenuSplitButtonProps extends React.Props<ContextualMenuSplitButton> {


should this extend contextualmenuitem?

You're right, we should.

dzearing · 2018-04-25T04:33:33Z

...ages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenuSplitButton.types.ts

+  /**
+   * CSS class to apply to the context menu.
+   */
+  classNames: IMenuItemClassNames;


Hmm. This is not the recommended pattern; could you change to getStyles when adding a component? Or is there a reason you're doing this?

The problem with classNames is that it can't be a function of the state, so you can't say have something which is styled according to the expanded state.

The only reason I did it this way was because I was passing an existing style, but since we're separating the split button from the menu, I think it makes sense to separate out the styles here.

@dzearing After thinking about this, we can split the split button part of the code out, however, we still need the className to get the styles for the ContextualMenuItems that we generate.

Specifically, I can split the move the split button to be its own style, but what should we do with the styles that we need to use for the ContextualMenuItem? For example className.icon and classname.checkmarkIcon, both of these are created in Contextual Menu and we currently pass it down to the contextual menu. Do we want to re-create it? Do we just pass it down? What are your thoughts?

that is totally fine... fwiw: you can still have getStyles, which takes those in.

getStyles={ props => { icon: className.icon, checkmarkIcon: classNames.checkmarkIcon }

One other thought is that it's entirely possible we change IGetStylesFunction to take in an object OR a function. That way if you don't care about theme or props to return some classes, you could just do that.

Huh, I see, so add it an as an addition got it. I'll do something like this for my next iteration on this. Thanks!

dzearing · 2018-04-25T04:52:17Z

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

+      split: true,
+    } as IContextualMenuItem;
+
+    const buttonProp = assign({}, getNativeProps(itemProps, buttonProperties), {


buttonProps

joschect · 2018-04-26T21:15:54Z

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

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


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

joschect · 2018-04-26T21:28:39Z

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

+        aria-checked={ item.isChecked || item.checked }
+        aria-posinset={ focusableElementIndex + 1 }
+        aria-setsize={ totalItemCount }
+        onMouseEnter={ this._onItemMouseEnter.bind({ ...item, subMenuProps: null, items: null }) }


Don't do the bind here, instead in move it to the function call in splitbutton like

const { onItemMouseEnter, item } = this.props; if (onItemMouseEnter) { onItemMouseEnter({...item, submenuProps: null}, items: null)}, ev, this._splitButton); }

…ouch

dzearing · 2018-04-25T04:53:14Z

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

+  }
+
+  private _renderSplitDivider(item: IContextualMenuItem) {
+    const getDividerClassnames = item.getSplitButtonVerticalDividerClassNames || getSplitButtonVerticalDividerClassNames;


classnames is 2 words (getDividerClassNames)

dzearing · 2018-04-25T04:53:56Z

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

+    } as IContextualMenuItem;
+    return (
+      <button {...getNativeProps(itemProps, buttonProperties) }>
+        <ChildrenRenderer data-is-focusable={ false } item={ itemProps } classNames={ classNames } index={ index } onCheckmarkClick={ hasCheckmarks && onItemClick ? onItemClick.bind(this, item) : undefined } hasIcons={ hasIcons } />


you have a bind here, and this is a really long line. Can you clean this up?

(at least line length.) I'm not sure about the bind.

dzearing · 2018-04-27T13:44:39Z

...ages/office-ui-fabric-react/src/components/ContextualMenu/ContextualMenuSplitButton.types.ts

+  /**
+   * CSS class to apply to the context menu.
+   */
+  classNames: IMenuItemClassNames;


that is totally fine... fwiw: you can still have getStyles, which takes those in.

getStyles={ props => { icon: className.icon, checkmarkIcon: classNames.checkmarkIcon }

One other thought is that it's entirely possible we change IGetStylesFunction to take in an object OR a function. That way if you don't care about theme or props to return some classes, you could just do that.

REDMOND\chiechan added 3 commits April 19, 2018 10:30

moved split button out of the contextual menu

2301c62

Removed all references to touch

922d51d

added change files

c7e22d3

chang47 requested a review from joschect as a code owner April 19, 2018 17:41

jspurlin reviewed Apr 19, 2018

View reviewed changes

REDMOND\chiechan added 2 commits April 19, 2018 14:23

solved style and updated test

e58ba9b

fixed problem with submenu appearing in the wrong target button

47e4cbd

joschect reviewed Apr 19, 2018

View reviewed changes

REDMOND\chiechan added 6 commits April 19, 2018 16:38

fixed style issue

4638505

removed unnecessary binds

277d73d

reverted removing items in functions; set correct event target for an…

254d3b7

…chor button menu focus

addressed style problems

ba5c45a

updated snapshot

87aa983

updated bundle size

bc3ae34

chang47 requested a review from dzearing as a code owner April 20, 2018 17:12

chang47 mentioned this pull request Apr 20, 2018

ComboBox/SplitButton Expand on Touch #4353

Merged

dzearing reviewed Apr 25, 2018

View reviewed changes

updated button styles with suggested changes

f0aa3d7

joschect reviewed Apr 26, 2018

View reviewed changes

REDMOND\chiechan added 2 commits April 26, 2018 17:15

separated the commands to be different autobind

8e16927

made target optional

ff88ae2

joschect approved these changes Apr 27, 2018

View reviewed changes

renamed actions to be a different name

c7a333a

dzearing closed this Apr 27, 2018

dzearing reopened this Apr 27, 2018

Merge branch 'master' into jspurlin/contextualMenuSplitButtonWithoutT…

8fba90e

…ouch

dzearing approved these changes Apr 27, 2018

View reviewed changes

REDMOND\chiechan added 5 commits April 27, 2018 07:23

removed unused prop fields

364fa11

updated snapsthos

dbf85ca

addressed comment to clean up code naming; bumped package size

a55c76c

resolved merge conflict

ee4d5ec

fixd lint error with closing tags

883e727

dzearing merged commit 175597d into microsoft:master Apr 27, 2018

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

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

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

Uh oh!

Conversation

chang47 commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chang47 commented Apr 19, 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!

chang47 Apr 20, 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!

christiango commented Apr 23, 2018

Uh oh!

chang47 commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzearing Apr 25, 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!

Choose a reason for hiding this comment

Uh oh!

dzearing Apr 27, 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!

chang47 Apr 27, 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!

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!

dzearing Apr 27, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chang47 commented Apr 19, 2018 •

edited

Loading

chang47 Apr 20, 2018 •

edited

Loading

chang47 commented Apr 24, 2018 •

edited

Loading

dzearing Apr 25, 2018 •

edited

Loading

dzearing Apr 27, 2018 •

edited

Loading

chang47 Apr 27, 2018 •

edited

Loading

dzearing Apr 27, 2018 •

edited

Loading