From fe8b3218097a43073d8f681c99f87a1e5535a64f Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Mon, 29 Jan 2018 18:23:16 -0800 Subject: [PATCH 01/16] remove MenuItem useSmartPositioning logic, as Popper.js handles it automatically!! --- .../core/src/common/abstractPureComponent.ts | 2 +- .../core/src/components/menu/menuItem.tsx | 195 +++--------------- packages/core/test/menu/menuTests.tsx | 172 --------------- .../examples/core-examples/menuExample.tsx | 2 +- 4 files changed, 35 insertions(+), 336 deletions(-) diff --git a/packages/core/src/common/abstractPureComponent.ts b/packages/core/src/common/abstractPureComponent.ts index efd5ee7681..8f9db40bc0 100644 --- a/packages/core/src/common/abstractPureComponent.ts +++ b/packages/core/src/common/abstractPureComponent.ts @@ -11,7 +11,7 @@ import { isNodeEnv } from "./utils"; * An abstract component that Blueprint components can extend * in order to add some common functionality like runtime props validation. */ -export abstract class AbstractPureComponent extends React.PureComponent { +export abstract class AbstractPureComponent extends React.PureComponent { /** Component displayName should be `public static`. This property exists to prevent incorrect usage. */ protected displayName: never; diff --git a/packages/core/src/components/menu/menuItem.tsx b/packages/core/src/components/menu/menuItem.tsx index b30b970a5f..6035aa1e75 100644 --- a/packages/core/src/components/menu/menuItem.tsx +++ b/packages/core/src/components/menu/menuItem.tsx @@ -13,7 +13,6 @@ import * as Errors from "../../common/errors"; import { Position } from "../../common/position"; import { IActionProps, ILinkProps } from "../../common/props"; import { IPopoverProps, Popover, PopoverInteractionKind } from "../popover/popover"; -import { Menu } from "./menu"; export interface IMenuItemProps extends IActionProps, ILinkProps { // override from IActionProps to make it required @@ -25,8 +24,8 @@ export interface IMenuItemProps extends IActionProps, ILinkProps { */ label?: string | JSX.Element; - /** Props to spread to `Popover`. Note that `content` cannot be changed. */ - popoverProps?: Partial & object; + /** Props to spread to `Popover`. The following props cannot be changed: `content`, `minimal`, `modifiers`. */ + popoverProps?: Partial; /** * Whether an enabled, non-submenu item should automatically close the @@ -40,59 +39,17 @@ export interface IMenuItemProps extends IActionProps, ILinkProps { * An alternative to providing `MenuItem` components as `children`. */ submenu?: IMenuItemProps[]; - - /** - * Width of `margin` from left or right edge of viewport. Submenus will - * flip to the other side if they come within this distance of that edge. - * This has no effect if omitted or if `useSmartPositioning` is set to `false`. - * Note that these values are not CSS properties; they are used in - * internal math to determine when to flip sides. - */ - submenuViewportMargin?: { left?: number; right?: number }; - - /** - * Whether a submenu popover will try to reposition itself - * if there isn't room for it in its current position. - * The popover opens right by default, but will try to flip - * left if not enough space. - * @default true - */ - useSmartPositioning?: boolean; -} - -export interface IMenuItemState { - /** Whether a submenu is opened to the left */ - alignLeft?: boolean; } -const REACT_CONTEXT_TYPES: React.ValidationMap = { - alignLeft: (obj: IMenuItemState, key: keyof IMenuItemState) => { - if (obj[key] != null && typeof obj[key] !== "boolean") { - return new Error("[Blueprint] MenuItem context alignLeft must be boolean"); - } - return undefined; - }, -}; - -export class MenuItem extends AbstractPureComponent { +export class MenuItem extends AbstractPureComponent { public static defaultProps: IMenuItemProps = { disabled: false, popoverProps: {}, shouldDismissPopover: true, - submenuViewportMargin: {}, text: "", - useSmartPositioning: true, }; public static displayName = "Blueprint2.MenuItem"; - public static contextTypes = REACT_CONTEXT_TYPES; - public static childContextTypes = REACT_CONTEXT_TYPES; - public context: IMenuItemState; - - public state: IMenuItemState = { - alignLeft: false, - }; - private liElement: HTMLElement; private popoverElement: HTMLElement; private refHandlers = { @@ -101,7 +58,7 @@ export class MenuItem extends AbstractPureComponent{label}; } - let content = ( + const content = ( ); - if (hasSubmenu) { - const submenuContent = {this.renderChildren()}; - const popoverClasses = classNames(Classes.MENU_SUBMENU, popoverProps.popoverClassName, { - // apply this class to make the popover anchor to the *left* - // side of the menu (setting Position.LEFT_TOP alone would still - // anchor the popover to the right edge of the target). - [Classes.ALIGN_LEFT]: this.state.alignLeft, - }); - - content = ( - - {content} - - ); - } - return (
  • - {content} + {hasSubmenu ? this.renderPopover(content) : content}
    • ); } - public getChildContext() { - return { alignLeft: this.state.alignLeft }; - } - protected validateProps(props: IMenuItemProps & { children?: React.ReactNode }) { if (props.children != null && props.submenu != null) { console.warn(Errors.MENU_WARN_CHILDREN_SUBMENU_MUTEX); } } - private handlePopoverDidOpen = () => { - if (this.props.useSmartPositioning) { - // Popper.js renders the popover in the DOM before relocating its - // position on the next tick. We need to rAF to wait for that to happen. - requestAnimationFrame(() => this.maybeAlignSubmenuLeft()); - } - }; + private renderPopover(content: JSX.Element) { + const { disabled, popoverProps } = this.props; + const popoverClasses = classNames(Classes.MENU_SUBMENU, popoverProps.popoverClassName); - private maybeAlignSubmenuLeft() { - if (this.popoverElement == null) { - return; - } + // NOTE: use .pt-menu directly because using Menu would start a new context tree and + // we'd have to pass through the submenu props. this is just simpler. + const submenuContent =
      {this.renderChildren()}
    ; - const submenuRect = this.popoverElement.getBoundingClientRect(); - const parentWidth = this.liElement.parentElement.getBoundingClientRect().width; - const adjustmentWidth = submenuRect.width + parentWidth; - - // this ensures that the left and right measurements represent a submenu opened to the right - let submenuLeft = submenuRect.left; - let submenuRight = submenuRect.right; - if (this.state.alignLeft) { - submenuLeft += adjustmentWidth; - submenuRight += adjustmentWidth; - } - - const { left = 0 } = this.props.submenuViewportMargin; - let { right = 0 } = this.props.submenuViewportMargin; - if ( - typeof document !== "undefined" && - typeof document.documentElement !== "undefined" && - Number(document.documentElement.clientWidth) - ) { - // we're in a browser context and the clientWidth is available, - // use it to set calculate 'right' - right = document.documentElement.clientWidth - right; - } - // uses context to prioritize the previous positioning - let alignLeft = this.context.alignLeft || false; - if (alignLeft) { - if (submenuLeft - adjustmentWidth <= left) { - alignLeft = false; - } - } else { - if (submenuRight >= right) { - alignLeft = true; - } - } - - this.setState({ alignLeft }); + return ( + + {content} + + ); } - private renderChildren = () => { + private renderChildren(): React.ReactNode { const { children, submenu } = this.props; - if (children != null) { - const childProps = this.cascadeProps(); - if (Object.keys(childProps).length === 0) { - return children; - } else { - return React.Children.map(children, (child: JSX.Element) => { - return React.cloneElement(child, childProps); - }); - } + return children; } else if (submenu != null) { - return submenu.map(this.cascadeProps).map(renderMenuItem); + return submenu.map(renderMenuItem); } else { - return undefined; + return null; } - }; - - /** - * Evalutes this.props and cascades prop values into new props when: - * - submenuViewportMargin is defined, but is undefined for the supplied input. - * - useSmartPositioning is false, but is undefined for the supplied input. - * @param {IMenuItemProps} newProps If supplied, object will be modified, otherwise, defaults to an empty object. - * @returns An object to be used as child props. - */ - private cascadeProps = (newProps: IMenuItemProps = ({} as any) as IMenuItemProps) => { - const { submenuViewportMargin, useSmartPositioning } = this.props; - - if (submenuViewportMargin != null && newProps.submenuViewportMargin == null) { - newProps.submenuViewportMargin = submenuViewportMargin; - } - if (useSmartPositioning === false && newProps.useSmartPositioning == null) { - newProps.useSmartPositioning = useSmartPositioning; - } - - return newProps; - }; + } } export function renderMenuItem(props: IMenuItemProps, key: string | number) { diff --git a/packages/core/test/menu/menuTests.tsx b/packages/core/test/menu/menuTests.tsx index 4cb1342e99..16d6515750 100644 --- a/packages/core/test/menu/menuTests.tsx +++ b/packages/core/test/menu/menuTests.tsx @@ -7,8 +7,6 @@ import { assert } from "chai"; import { mount, shallow, ShallowWrapper } from "enzyme"; import * as React from "react"; -import * as ReactDOM from "react-dom"; -import * as TestUtils from "react-dom/test-utils"; import { spy, stub } from "sinon"; import { MENU_WARN_CHILDREN_SUBMENU_MUTEX } from "../../src/common/errors"; @@ -134,176 +132,6 @@ describe("MenuItem", () => { ); assert.notStrictEqual(wrapper.find(Popover).prop("content"), popoverProps.content); }); - - /** - * If debugging `gulp karma-unit-core` in an actual browser, - * the window size should be set to 480 width x 800 height (identical to PhantomJS). - */ - describe("dynamic layout:", () => { - let childContainer: HTMLElement; - let menuItem: MenuItem; - - before(() => { - childContainer = document.createElement("div"); - - // karma's window size is by default 480px, so need to confine space - childContainer.style.width = "1px"; - childContainer.style.marginLeft = `${document.documentElement.clientWidth - 30}px`; - document.documentElement.appendChild(childContainer); - }); - - afterEach(() => ReactDOM.unmountComponentAtNode(childContainer)); - - it("children can display left", done => { - menuItem = mountMenuItem( - { iconName: "style", text: "Style" }, - // use a fragment instead of an array, so we don't have to key each item - - - - - , - ); - hoverOverTarget(0, () => { - assert.isNotNull(childContainer.querySelector(`.${Classes.ALIGN_LEFT}`)); - done(); - }); - }); - - it("useSmartPositioning=false prevents display left behavior", done => { - menuItem = mountMenuItem( - { iconName: "style", text: "Style", useSmartPositioning: false }, - - - - - , - ); - hoverOverTarget(0, () => { - assert.isNotNull(childContainer.querySelector(`.${Classes.OVERLAY_OPEN}`)); - assert.isNull(childContainer.querySelector(`.${Classes.ALIGN_LEFT}`)); - done(); - }); - }); - - // TODO (clewis): Commented this out during the Popover2 => Popover refactor. Get this working again. - it.skip("children will continue displaying in the same direction if possible", done => { - menuItem = mountMenuItem( - { iconName: "style", text: "Style" }, - - - - - - - - - - , - ); - hoverOverTarget(0, () => { - hoverOverTarget(4, () => { - assertClassNameCount(Classes.ALIGN_LEFT, 2); - done(); - }); - }); - }); - - // TODO (clewis): Commented this out during the Popover2 => Popover refactor. Get this working again. - it.skip("children will flip direction after no more room in the existing direction", done => { - menuItem = mountMenuItem( - { iconName: "style", text: "Style" }, - - - - - - - - - - - - - - - , - ); - hoverOverTarget(0, () => { - hoverOverTarget(4, () => { - hoverOverTarget(8, () => { - assertClassNameCount(Classes.OVERLAY_OPEN, 3); - assertClassNameCount(Classes.ALIGN_LEFT, 2); - done(); - }); - }); - }); - }); - - it("submenu as props can display left", done => { - const items: IMenuItemProps[] = [ - { iconName: "align-left", text: "Align Left" }, - { iconName: "align-center", text: "Align Center" }, - { iconName: "align-right", text: "Align Right" }, - ]; - menuItem = mountMenuItem({ iconName: "align-left", text: "Alignment", submenu: items }); - hoverOverTarget(0, () => { - assert.isNotNull(childContainer.querySelector(`.${Classes.ALIGN_LEFT}`)); - done(); - }); - }); - - it("submenu as props can inherit submenuViewportMargin prop from parent", done => { - const items: IMenuItemProps[] = [ - { iconName: "align-left", text: "Align Left" }, - { iconName: "align-center", text: "Align Center" }, - { - iconName: "align-right", - submenu: [ - { iconName: "align-left", text: "Align Left" }, - { iconName: "align-center", text: "Align Center" }, - { iconName: "align-right", text: "Align Right" }, - ], - text: "Align Right", - }, - ]; - menuItem = mountMenuItem({ - iconName: "align-left", - submenu: items, - submenuViewportMargin: { left: 150 }, - text: "Alignment", - }); - hoverOverTarget(0, () => { - hoverOverTarget(3, () => { - assertClassNameCount(Classes.ALIGN_LEFT, 1); - assertClassNameCount(Classes.OVERLAY_OPEN, 2); - done(); - }); - }); - }); - - function hoverOverTarget(index: number, handler: () => void, timeout = 150) { - // if popover's hoverOpenDelay !== 0 this function needs to also be slowed down; - // otherwise, the submenu will not have been opened yet for the test - const a = TestUtils.scryRenderedDOMComponentsWithTag(menuItem, "a")[index]; - // TODO: (BLUEPRINT-536) try and find an alternative to SimulateNative - (TestUtils as any).SimulateNative.mouseOver(a); - return setTimeout(handler, timeout); - } - - function assertClassNameCount(className: string, count: number) { - assert.lengthOf(childContainer.querySelectorAll(`.${className}`), count, `${count}x .${className}`); - } - - function mountMenuItem(props?: IMenuItemProps, childItems?: React.ReactNode) { - return ReactDOM.render( - - {childItems} - , - childContainer, - ) as MenuItem; - } - }); }); describe("MenuDivider", () => { diff --git a/packages/docs-app/src/examples/core-examples/menuExample.tsx b/packages/docs-app/src/examples/core-examples/menuExample.tsx index 76900c8112..2215469163 100644 --- a/packages/docs-app/src/examples/core-examples/menuExample.tsx +++ b/packages/docs-app/src/examples/core-examples/menuExample.tsx @@ -37,7 +37,7 @@ export class MenuExample extends BaseExample<{}> { - + From 46fe7542559ee57be4fb679cba8a684cfb432377 Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Mon, 29 Jan 2018 18:25:30 -0800 Subject: [PATCH 02/16] Add submenuBoundaryElement, submenuBoundaryPadding props to Menu. cascade to MenuItem via context fn (better than cloning children). --- packages/core/src/components/menu/context.ts | 24 +++++++++++++ packages/core/src/components/menu/menu.tsx | 36 +++++++++++++++++++ .../core/src/components/menu/menuItem.tsx | 8 +++++ packages/core/test/menu/menuTests.tsx | 15 ++++++++ 4 files changed, 83 insertions(+) create mode 100644 packages/core/src/components/menu/context.ts diff --git a/packages/core/src/components/menu/context.ts b/packages/core/src/components/menu/context.ts new file mode 100644 index 0000000000..8298cb0206 --- /dev/null +++ b/packages/core/src/components/menu/context.ts @@ -0,0 +1,24 @@ +/* + * Copyright 2018 Palantir Technologies, Inc. All rights reserved. + * + * Licensed under the terms of the LICENSE file distributed with this project. + */ + +import { isFunction } from "../../common/utils"; + +export interface IMenuItemContext { + getSubmenuPopperModifiers?(): Popper.Modifiers; +} + +export const MenuItemContextTypes: React.ValidationMap = { + getSubmenuPopperModifiers: assertFunctionProp, +}; + +// simple alternative to prop-types dependency +function assertFunctionProp(obj: T, key: keyof T) { + // context method is optional, so allow nulls + if (obj[key] == null || isFunction(obj[key])) { + return undefined; + } + return new Error(`[Blueprint] context ${key} must be function. received ${typeof obj[key]}.`); +} diff --git a/packages/core/src/components/menu/menu.tsx b/packages/core/src/components/menu/menu.tsx index 64d33a7289..cb2ae011a3 100644 --- a/packages/core/src/components/menu/menu.tsx +++ b/packages/core/src/components/menu/menu.tsx @@ -9,14 +9,38 @@ import * as React from "react"; import * as Classes from "../../common/classes"; import { IProps } from "../../common/props"; +import { IMenuItemContext, MenuItemContextTypes } from "./context"; export interface IMenuProps extends IProps { /** Ref handler that receives the HTML `
      ` element backing this component. */ ulRef?: (ref: HTMLUListElement) => any; + + /** + * Boundary element for position of submenu. If submenu reaches the edge of the boundary, + * it will flip to the other side. + * This prop will apply to all `MenuItem`s inside this `Menu`, via React context. + * @see https://popper.js.org/popper-documentation.html#modifiers..flip.boundariesElement + * @default "viewport" + */ + submenuBoundaryElement?: "scrollParent" | "viewport" | "window" | Element; + + /** + * Pixel width of minimum distance between boundary element and submenu content. + * Submenus will flip to the other side if they come within this distance of the boundary. + * This prop will apply to all `MenuItem`s inside this `Menu`, via React context. + * @see https://popper.js.org/popper-documentation.html#modifiers..preventOverflow.padding + * @default 5 + */ + submenuBoundaryPadding?: number; } export class Menu extends React.Component { + public static childContextTypes = MenuItemContextTypes; public static displayName = "Blueprint2.Menu"; + public static defaultProps: IMenuProps = { + submenuBoundaryElement: "viewport", + submenuBoundaryPadding: 5, + }; public render() { return ( @@ -25,4 +49,16 @@ export class Menu extends React.Component {
    ); } + + public getChildContext(): IMenuItemContext { + return { + getSubmenuPopperModifiers: () => { + const { submenuBoundaryElement: boundariesElement, submenuBoundaryPadding: padding } = this.props; + return { + flip: { boundariesElement, padding }, + preventOverflow: { boundariesElement, padding }, + }; + }, + }; + } } diff --git a/packages/core/src/components/menu/menuItem.tsx b/packages/core/src/components/menu/menuItem.tsx index 6035aa1e75..22ec3166aa 100644 --- a/packages/core/src/components/menu/menuItem.tsx +++ b/packages/core/src/components/menu/menuItem.tsx @@ -12,7 +12,9 @@ import * as Classes from "../../common/classes"; import * as Errors from "../../common/errors"; import { Position } from "../../common/position"; import { IActionProps, ILinkProps } from "../../common/props"; +import { safeInvoke } from "../../common/utils"; import { IPopoverProps, Popover, PopoverInteractionKind } from "../popover/popover"; +import { IMenuItemContext, MenuItemContextTypes } from "./context"; export interface IMenuItemProps extends IActionProps, ILinkProps { // override from IActionProps to make it required @@ -50,6 +52,9 @@ export class MenuItem extends AbstractPureComponent { }; public static displayName = "Blueprint2.MenuItem"; + public static contextTypes = MenuItemContextTypes; + public context: IMenuItemContext; + private liElement: HTMLElement; private popoverElement: HTMLElement; private refHandlers = { @@ -109,6 +114,8 @@ export class MenuItem extends AbstractPureComponent { private renderPopover(content: JSX.Element) { const { disabled, popoverProps } = this.props; const popoverClasses = classNames(Classes.MENU_SUBMENU, popoverProps.popoverClassName); + // getSubmenuPopperModifiers will not be defined if `MenuItem` used outside a `Menu`. + const popoverModifiers = safeInvoke(this.context.getSubmenuPopperModifiers); // NOTE: use .pt-menu directly because using Menu would start a new context tree and // we'd have to pass through the submenu props. this is just simpler. @@ -124,6 +131,7 @@ export class MenuItem extends AbstractPureComponent { {...popoverProps} content={submenuContent} minimal={true} + modifiers={popoverModifiers} popoverClassName={popoverClasses} popoverRef={this.refHandlers.popover} > diff --git a/packages/core/test/menu/menuTests.tsx b/packages/core/test/menu/menuTests.tsx index 16d6515750..9ed65c1aff 100644 --- a/packages/core/test/menu/menuTests.tsx +++ b/packages/core/test/menu/menuTests.tsx @@ -158,6 +158,21 @@ describe("Menu", () => { assert.isTrue(menu.hasClass(Classes.MENU)); assert.lengthOf(menu.find(MenuItem), 1); }); + + it("boundary props are exposed to children via context method", () => { + const modifiers = { boundariesElement: "window" as "window", padding: 100 }; + const menu = mount( + + + + + + , + ); + const { flip, preventOverflow } = menu.find(Popover).prop("modifiers"); + assert.deepEqual(flip, modifiers); + assert.deepEqual(preventOverflow, modifiers); + }); }); function findSubmenu(wrapper: ShallowWrapper) { From f678135fd097be5927df50af3f7b082d6c2706eb Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Mon, 29 Jan 2018 18:27:31 -0800 Subject: [PATCH 03/16] docs --- packages/core/src/components/menu/menu.md | 33 +++++++++++++---------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/packages/core/src/components/menu/menu.md b/packages/core/src/components/menu/menu.md index 1bc7e96544..d5d78ea1d1 100644 --- a/packages/core/src/components/menu/menu.md +++ b/packages/core/src/components/menu/menu.md @@ -81,27 +81,32 @@ Use `MenuDivider` to separate menu sections. Optionally, add a title to the divi To add a submenu to a `Menu`, simply nest `MenuItem`s within another `MenuItem`. The submenu opens to the right of its parent by default, but will adjust and flip to the left if -there is not enough room to the right. +there is not enough room to the right. `Menu` provides two `submenu` props to adjust this flipping behavior: +you can customize the boundary element and the padding within that boundary element. ```jsx - - - - - + + + + + + + ``` Alternatively, you can pass an array of `IMenuItemProps` to the `submenu` prop: ```jsx -React.createElement(MenuItem, { - submenu: [ - { text: "Child one" }, - { text: "Child two" }, - { text: "Child three" }, - ], - text: "parent", -}); +React.createElement(Menu, {}, + React.createElement(MenuItem, { + submenu: [ + { text: "Child one" }, + { text: "Child two" }, + { text: "Child three" }, + ], + text: "parent", + }), +); ```
    From aef958edbd5f14dbe02c6ee44ec7f01e2a78b4df Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Mon, 29 Jan 2018 18:32:19 -0800 Subject: [PATCH 04/16] more docs --- packages/core/src/components/menu/menu.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/core/src/components/menu/menu.md b/packages/core/src/components/menu/menu.md index d5d78ea1d1..e07d9ebf03 100644 --- a/packages/core/src/components/menu/menu.md +++ b/packages/core/src/components/menu/menu.md @@ -81,8 +81,12 @@ Use `MenuDivider` to separate menu sections. Optionally, add a title to the divi To add a submenu to a `Menu`, simply nest `MenuItem`s within another `MenuItem`. The submenu opens to the right of its parent by default, but will adjust and flip to the left if -there is not enough room to the right. `Menu` provides two `submenu` props to adjust this flipping behavior: -you can customize the boundary element and the padding within that boundary element. +there is not enough room to the right. + +`Menu` provides two `submenu` props to adjust this flipping behavior: you can customize the boundary element +and the padding within that boundary element. Note that a `MenuItem` _must be inside_ a `Menu` element +for these props to affect the submenus. On standalone `MenuItem`s (without a parent `Menu`, an anti-pattern) you can +use `popoverProps.modifiers` directly to achieve the same result. ```jsx From e23f78c5d6437f092dd5e877666aace2b533e139 Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 30 Jan 2018 13:10:17 -0800 Subject: [PATCH 05/16] use prop-types, add dependency --- package.json | 1 + packages/core/package.json | 1 + packages/core/src/components/menu/context.ts | 13 ++----------- packages/docs-theme/package.json | 1 + packages/docs-theme/src/common/context.ts | 18 +++++------------- yarn.lock | 4 ++++ 6 files changed, 14 insertions(+), 24 deletions(-) diff --git a/package.json b/package.json index 277acfcda1..216e5a3268 100644 --- a/package.json +++ b/package.json @@ -40,6 +40,7 @@ "@types/enzyme": "^3.1.6", "@types/enzyme-adapter-react-16": "^1.0.1", "@types/mocha": "^2.2.46", + "@types/prop-types": "^15.5.2", "@types/react": "^16.0.34", "@types/react-dom": "^16.0.3", "@types/react-transition-group": "^2.0.6", diff --git a/packages/core/package.json b/packages/core/package.json index 2b36c0faf7..21c1e03335 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -37,6 +37,7 @@ "dom4": "^2.0.1", "normalize.css": "^7.0.0", "popper.js": "^1.12.6", + "prop-types": "^15.6.0", "react-popper": "~0.7.4", "tslib": "^1.5.0" }, diff --git a/packages/core/src/components/menu/context.ts b/packages/core/src/components/menu/context.ts index 8298cb0206..085598338f 100644 --- a/packages/core/src/components/menu/context.ts +++ b/packages/core/src/components/menu/context.ts @@ -4,21 +4,12 @@ * Licensed under the terms of the LICENSE file distributed with this project. */ -import { isFunction } from "../../common/utils"; +import { func } from "prop-types"; export interface IMenuItemContext { getSubmenuPopperModifiers?(): Popper.Modifiers; } export const MenuItemContextTypes: React.ValidationMap = { - getSubmenuPopperModifiers: assertFunctionProp, + getSubmenuPopperModifiers: func, }; - -// simple alternative to prop-types dependency -function assertFunctionProp(obj: T, key: keyof T) { - // context method is optional, so allow nulls - if (obj[key] == null || isFunction(obj[key])) { - return undefined; - } - return new Error(`[Blueprint] context ${key} must be function. received ${typeof obj[key]}.`); -} diff --git a/packages/docs-theme/package.json b/packages/docs-theme/package.json index 6d88603b21..c39d880e1b 100644 --- a/packages/docs-theme/package.json +++ b/packages/docs-theme/package.json @@ -29,6 +29,7 @@ "classnames": "^2.2", "documentalist": "^1.0.0-beta.4", "fuzzaldrin-plus": "^0.5.0", + "prop-types": "^15.6.0", "tslib": "^1.5.0" }, "devDependencies": { diff --git a/packages/docs-theme/src/common/context.ts b/packages/docs-theme/src/common/context.ts index 3be4922367..cdb13fc5f3 100644 --- a/packages/docs-theme/src/common/context.ts +++ b/packages/docs-theme/src/common/context.ts @@ -4,7 +4,6 @@ * Licensed under the terms of the LICENSE file distributed with this project. */ -import { Utils } from "@blueprintjs/core"; import { IBlock, IKssPluginData, @@ -12,6 +11,7 @@ import { ITsDocBase, ITypescriptPluginData, } from "documentalist/dist/client"; +import { func } from "prop-types"; /** This docs theme requires Markdown data and optionally supports Typescript and KSS data. */ export type IDocsData = IMarkdownPluginData & (ITypescriptPluginData | {}) & (IKssPluginData | {}); @@ -62,16 +62,8 @@ export interface IDocumentationContext { * ``` */ export const DocumentationContextTypes: React.ValidationMap = { - getDocsData: assertFunctionProp, - renderBlock: assertFunctionProp, - renderType: assertFunctionProp, - renderViewSourceLinkText: assertFunctionProp, + getDocsData: func.isRequired, + renderBlock: func.isRequired, + renderType: func.isRequired, + renderViewSourceLinkText: func.isRequired, }; - -// simple alternative to prop-types dependency -function assertFunctionProp(obj: T, key: keyof T) { - if (obj[key] != null && Utils.isFunction(obj[key])) { - return undefined; - } - return new Error(`[Blueprint] Documentation context ${key} must be function.`); -} diff --git a/yarn.lock b/yarn.lock index f8a40df812..3b993bbfaf 100644 --- a/yarn.lock +++ b/yarn.lock @@ -83,6 +83,10 @@ version "9.3.0" resolved "https://registry.yarnpkg.com/@types/node/-/node-9.3.0.tgz#3a129cda7c4e5df2409702626892cb4b96546dd5" +"@types/prop-types@^15.5.2": + version "15.5.2" + resolved "https://registry.yarnpkg.com/@types/prop-types/-/prop-types-15.5.2.tgz#3c6b8dceb2906cc87fe4358e809f9d20c8d59be1" + "@types/react-dom@^16.0.3": version "16.0.3" resolved "https://registry.yarnpkg.com/@types/react-dom/-/react-dom-16.0.3.tgz#8accad7eabdab4cca3e1a56f5ccb57de2da0ff64" From a796819a4e71b34a2d47352968a71b035afe054a Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 30 Jan 2018 13:21:20 -0800 Subject: [PATCH 06/16] refactor MenuItem render. no children => no popover --- .../core/src/components/menu/menuItem.tsx | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/packages/core/src/components/menu/menuItem.tsx b/packages/core/src/components/menu/menuItem.tsx index 22ec3166aa..297f0ecf0f 100644 --- a/packages/core/src/components/menu/menuItem.tsx +++ b/packages/core/src/components/menu/menuItem.tsx @@ -63,11 +63,11 @@ export class MenuItem extends AbstractPureComponent { }; public render() { - const { children, disabled, label, submenu } = this.props; - const hasSubmenu = children != null || submenu != null; - const liClasses = classNames({ - [Classes.MENU_SUBMENU]: hasSubmenu, - }); + const { disabled, label } = this.props; + const submenuChildren = this.renderSubmenuChildren(); + const hasSubmenu = submenuChildren != null; + + const liClasses = classNames({ [Classes.MENU_SUBMENU]: hasSubmenu }); const anchorClasses = classNames( Classes.MENU_ITEM, Classes.intentClass(this.props.intent), @@ -80,12 +80,7 @@ export class MenuItem extends AbstractPureComponent { this.props.className, ); - let labelElement: JSX.Element; - if (label != null) { - labelElement = {label}; - } - - const content = ( + const target = ( { tabIndex={disabled ? undefined : 0} target={this.props.target} > - {labelElement} + {label && {label}} {this.props.text} ); return (
  • - {hasSubmenu ? this.renderPopover(content) : content} + {this.maybeRenderPopover(target, submenuChildren)}
    • ); } @@ -111,15 +106,19 @@ export class MenuItem extends AbstractPureComponent { } } - private renderPopover(content: JSX.Element) { + private maybeRenderPopover(target: JSX.Element, children?: React.ReactNode) { const { disabled, popoverProps } = this.props; + if (children == null) { + return target; + } + const popoverClasses = classNames(Classes.MENU_SUBMENU, popoverProps.popoverClassName); // getSubmenuPopperModifiers will not be defined if `MenuItem` used outside a `Menu`. const popoverModifiers = safeInvoke(this.context.getSubmenuPopperModifiers); // NOTE: use .pt-menu directly because using Menu would start a new context tree and // we'd have to pass through the submenu props. this is just simpler. - const submenuContent =
      {this.renderChildren()}
    ; + const submenuContent =
      {children}
    ; return ( { popoverClassName={popoverClasses} popoverRef={this.refHandlers.popover} > - {content} + {target} ); } - private renderChildren(): React.ReactNode { + private renderSubmenuChildren(): React.ReactNode { const { children, submenu } = this.props; if (children != null) { return children; From b770995ece7ce4f1021750e95f8db2c19d3ce459 Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Tue, 30 Jan 2018 13:34:19 -0800 Subject: [PATCH 07/16] use Menu and pass context props --- packages/core/src/components/menu/menuItem.tsx | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/core/src/components/menu/menuItem.tsx b/packages/core/src/components/menu/menuItem.tsx index 297f0ecf0f..ef183656a2 100644 --- a/packages/core/src/components/menu/menuItem.tsx +++ b/packages/core/src/components/menu/menuItem.tsx @@ -15,6 +15,7 @@ import { IActionProps, ILinkProps } from "../../common/props"; import { safeInvoke } from "../../common/utils"; import { IPopoverProps, Popover, PopoverInteractionKind } from "../popover/popover"; import { IMenuItemContext, MenuItemContextTypes } from "./context"; +import { IMenuProps, Menu } from "./menu"; export interface IMenuItemProps extends IActionProps, ILinkProps { // override from IActionProps to make it required @@ -116,9 +117,15 @@ export class MenuItem extends AbstractPureComponent { // getSubmenuPopperModifiers will not be defined if `MenuItem` used outside a `Menu`. const popoverModifiers = safeInvoke(this.context.getSubmenuPopperModifiers); - // NOTE: use .pt-menu directly because using Menu would start a new context tree and - // we'd have to pass through the submenu props. this is just simpler. - const submenuContent =
      {children}
    ; + // Must pass parent `Menu` context props down to nested `Menu` + const menuProps: IMenuProps = + popoverModifiers == null + ? {} + : { + submenuBoundaryElement: popoverModifiers.flip.boundariesElement, + submenuBoundaryPadding: popoverModifiers.flip.padding, + }; + const submenuContent = {children}; return ( Date: Wed, 31 Jan 2018 11:38:20 -0800 Subject: [PATCH 08/16] remove Menu props and context stuff --- packages/core/src/components/menu/context.ts | 15 -------- packages/core/src/components/menu/menu.tsx | 36 ------------------- .../core/src/components/menu/menuItem.tsx | 28 +++++---------- 3 files changed, 9 insertions(+), 70 deletions(-) delete mode 100644 packages/core/src/components/menu/context.ts diff --git a/packages/core/src/components/menu/context.ts b/packages/core/src/components/menu/context.ts deleted file mode 100644 index 085598338f..0000000000 --- a/packages/core/src/components/menu/context.ts +++ /dev/null @@ -1,15 +0,0 @@ -/* - * Copyright 2018 Palantir Technologies, Inc. All rights reserved. - * - * Licensed under the terms of the LICENSE file distributed with this project. - */ - -import { func } from "prop-types"; - -export interface IMenuItemContext { - getSubmenuPopperModifiers?(): Popper.Modifiers; -} - -export const MenuItemContextTypes: React.ValidationMap = { - getSubmenuPopperModifiers: func, -}; diff --git a/packages/core/src/components/menu/menu.tsx b/packages/core/src/components/menu/menu.tsx index cb2ae011a3..64d33a7289 100644 --- a/packages/core/src/components/menu/menu.tsx +++ b/packages/core/src/components/menu/menu.tsx @@ -9,38 +9,14 @@ import * as React from "react"; import * as Classes from "../../common/classes"; import { IProps } from "../../common/props"; -import { IMenuItemContext, MenuItemContextTypes } from "./context"; export interface IMenuProps extends IProps { /** Ref handler that receives the HTML `
      ` element backing this component. */ ulRef?: (ref: HTMLUListElement) => any; - - /** - * Boundary element for position of submenu. If submenu reaches the edge of the boundary, - * it will flip to the other side. - * This prop will apply to all `MenuItem`s inside this `Menu`, via React context. - * @see https://popper.js.org/popper-documentation.html#modifiers..flip.boundariesElement - * @default "viewport" - */ - submenuBoundaryElement?: "scrollParent" | "viewport" | "window" | Element; - - /** - * Pixel width of minimum distance between boundary element and submenu content. - * Submenus will flip to the other side if they come within this distance of the boundary. - * This prop will apply to all `MenuItem`s inside this `Menu`, via React context. - * @see https://popper.js.org/popper-documentation.html#modifiers..preventOverflow.padding - * @default 5 - */ - submenuBoundaryPadding?: number; } export class Menu extends React.Component { - public static childContextTypes = MenuItemContextTypes; public static displayName = "Blueprint2.Menu"; - public static defaultProps: IMenuProps = { - submenuBoundaryElement: "viewport", - submenuBoundaryPadding: 5, - }; public render() { return ( @@ -49,16 +25,4 @@ export class Menu extends React.Component {
    ); } - - public getChildContext(): IMenuItemContext { - return { - getSubmenuPopperModifiers: () => { - const { submenuBoundaryElement: boundariesElement, submenuBoundaryPadding: padding } = this.props; - return { - flip: { boundariesElement, padding }, - preventOverflow: { boundariesElement, padding }, - }; - }, - }; - } } diff --git a/packages/core/src/components/menu/menuItem.tsx b/packages/core/src/components/menu/menuItem.tsx index ef183656a2..2eae33369a 100644 --- a/packages/core/src/components/menu/menuItem.tsx +++ b/packages/core/src/components/menu/menuItem.tsx @@ -14,8 +14,7 @@ import { Position } from "../../common/position"; import { IActionProps, ILinkProps } from "../../common/props"; import { safeInvoke } from "../../common/utils"; import { IPopoverProps, Popover, PopoverInteractionKind } from "../popover/popover"; -import { IMenuItemContext, MenuItemContextTypes } from "./context"; -import { IMenuProps, Menu } from "./menu"; +import { Menu } from "./menu"; export interface IMenuItemProps extends IActionProps, ILinkProps { // override from IActionProps to make it required @@ -53,14 +52,11 @@ export class MenuItem extends AbstractPureComponent { }; public static displayName = "Blueprint2.MenuItem"; - public static contextTypes = MenuItemContextTypes; - public context: IMenuItemContext; - private liElement: HTMLElement; private popoverElement: HTMLElement; private refHandlers = { li: (ref: HTMLElement) => (this.liElement = ref), - popover: (ref: HTMLElement) => (this.popoverElement = ref), + popover: (ref: HTMLDivElement) => (this.popoverElement = ref), }; public render() { @@ -114,18 +110,7 @@ export class MenuItem extends AbstractPureComponent { } const popoverClasses = classNames(Classes.MENU_SUBMENU, popoverProps.popoverClassName); - // getSubmenuPopperModifiers will not be defined if `MenuItem` used outside a `Menu`. - const popoverModifiers = safeInvoke(this.context.getSubmenuPopperModifiers); - - // Must pass parent `Menu` context props down to nested `Menu` - const menuProps: IMenuProps = - popoverModifiers == null - ? {} - : { - submenuBoundaryElement: popoverModifiers.flip.boundariesElement, - submenuBoundaryPadding: popoverModifiers.flip.padding, - }; - const submenuContent = {children}; + const submenuContent = {children}; return ( { enforceFocus={false} hoverCloseDelay={0} interactionKind={PopoverInteractionKind.HOVER} + modifiers={SUBMENU_POPOVER_MODIFIERS} position={Position.RIGHT_TOP} {...popoverProps} content={submenuContent} minimal={true} - modifiers={popoverModifiers} popoverClassName={popoverClasses} popoverRef={this.refHandlers.popover} > @@ -161,3 +146,8 @@ export class MenuItem extends AbstractPureComponent { export function renderMenuItem(props: IMenuItemProps, key: string | number) { return ; } + +const SUBMENU_POPOVER_MODIFIERS: Popper.Modifiers = { + flip: { boundariesElement: "viewport", padding: 5 }, + preventOverflow: { boundariesElement: "viewport", padding: 5 }, +}; From 9800ef288a845d433798465c20d9c07868c16421 Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Wed, 31 Jan 2018 11:40:43 -0800 Subject: [PATCH 09/16] remove unused refs, final refactors --- .../core/src/components/menu/menuItem.tsx | 35 +++++-------------- 1 file changed, 8 insertions(+), 27 deletions(-) diff --git a/packages/core/src/components/menu/menuItem.tsx b/packages/core/src/components/menu/menuItem.tsx index 2eae33369a..33f3f366a9 100644 --- a/packages/core/src/components/menu/menuItem.tsx +++ b/packages/core/src/components/menu/menuItem.tsx @@ -12,7 +12,6 @@ import * as Classes from "../../common/classes"; import * as Errors from "../../common/errors"; import { Position } from "../../common/position"; import { IActionProps, ILinkProps } from "../../common/props"; -import { safeInvoke } from "../../common/utils"; import { IPopoverProps, Popover, PopoverInteractionKind } from "../popover/popover"; import { Menu } from "./menu"; @@ -52,13 +51,6 @@ export class MenuItem extends AbstractPureComponent { }; public static displayName = "Blueprint2.MenuItem"; - private liElement: HTMLElement; - private popoverElement: HTMLElement; - private refHandlers = { - li: (ref: HTMLElement) => (this.liElement = ref), - popover: (ref: HTMLDivElement) => (this.popoverElement = ref), - }; - public render() { const { disabled, label } = this.props; const submenuChildren = this.renderSubmenuChildren(); @@ -90,11 +82,7 @@ export class MenuItem extends AbstractPureComponent { ); - return ( -
  • - {this.maybeRenderPopover(target, submenuChildren)} -
    • - ); + return
  • {this.maybeRenderPopover(target, submenuChildren)}
    • ; } protected validateProps(props: IMenuItemProps & { children?: React.ReactNode }) { @@ -105,14 +93,9 @@ export class MenuItem extends AbstractPureComponent { private maybeRenderPopover(target: JSX.Element, children?: React.ReactNode) { const { disabled, popoverProps } = this.props; - if (children == null) { - return target; - } - - const popoverClasses = classNames(Classes.MENU_SUBMENU, popoverProps.popoverClassName); - const submenuContent = {children}; - - return ( + return children == null ? ( + target + ) : ( { modifiers={SUBMENU_POPOVER_MODIFIERS} position={Position.RIGHT_TOP} {...popoverProps} - content={submenuContent} + content={{children}} minimal={true} - popoverClassName={popoverClasses} - popoverRef={this.refHandlers.popover} - > - {target} - + popoverClassName={classNames(Classes.MENU_SUBMENU, popoverProps.popoverClassName)} + target={target} + /> ); } From c7bd0acd799ee7ace8dc166b88d4d74ff485d7f3 Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Wed, 31 Jan 2018 11:41:59 -0800 Subject: [PATCH 10/16] revert props-types change --- package.json | 1 - packages/core/package.json | 1 - packages/docs-theme/package.json | 1 - packages/docs-theme/src/common/context.ts | 18 +++++++++++++----- yarn.lock | 4 ---- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/package.json b/package.json index 216e5a3268..277acfcda1 100644 --- a/package.json +++ b/package.json @@ -40,7 +40,6 @@ "@types/enzyme": "^3.1.6", "@types/enzyme-adapter-react-16": "^1.0.1", "@types/mocha": "^2.2.46", - "@types/prop-types": "^15.5.2", "@types/react": "^16.0.34", "@types/react-dom": "^16.0.3", "@types/react-transition-group": "^2.0.6", diff --git a/packages/core/package.json b/packages/core/package.json index 21c1e03335..2b36c0faf7 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -37,7 +37,6 @@ "dom4": "^2.0.1", "normalize.css": "^7.0.0", "popper.js": "^1.12.6", - "prop-types": "^15.6.0", "react-popper": "~0.7.4", "tslib": "^1.5.0" }, diff --git a/packages/docs-theme/package.json b/packages/docs-theme/package.json index c39d880e1b..6d88603b21 100644 --- a/packages/docs-theme/package.json +++ b/packages/docs-theme/package.json @@ -29,7 +29,6 @@ "classnames": "^2.2", "documentalist": "^1.0.0-beta.4", "fuzzaldrin-plus": "^0.5.0", - "prop-types": "^15.6.0", "tslib": "^1.5.0" }, "devDependencies": { diff --git a/packages/docs-theme/src/common/context.ts b/packages/docs-theme/src/common/context.ts index cdb13fc5f3..3be4922367 100644 --- a/packages/docs-theme/src/common/context.ts +++ b/packages/docs-theme/src/common/context.ts @@ -4,6 +4,7 @@ * Licensed under the terms of the LICENSE file distributed with this project. */ +import { Utils } from "@blueprintjs/core"; import { IBlock, IKssPluginData, @@ -11,7 +12,6 @@ import { ITsDocBase, ITypescriptPluginData, } from "documentalist/dist/client"; -import { func } from "prop-types"; /** This docs theme requires Markdown data and optionally supports Typescript and KSS data. */ export type IDocsData = IMarkdownPluginData & (ITypescriptPluginData | {}) & (IKssPluginData | {}); @@ -62,8 +62,16 @@ export interface IDocumentationContext { * ``` */ export const DocumentationContextTypes: React.ValidationMap = { - getDocsData: func.isRequired, - renderBlock: func.isRequired, - renderType: func.isRequired, - renderViewSourceLinkText: func.isRequired, + getDocsData: assertFunctionProp, + renderBlock: assertFunctionProp, + renderType: assertFunctionProp, + renderViewSourceLinkText: assertFunctionProp, }; + +// simple alternative to prop-types dependency +function assertFunctionProp(obj: T, key: keyof T) { + if (obj[key] != null && Utils.isFunction(obj[key])) { + return undefined; + } + return new Error(`[Blueprint] Documentation context ${key} must be function.`); +} diff --git a/yarn.lock b/yarn.lock index 3b993bbfaf..f8a40df812 100644 --- a/yarn.lock +++ b/yarn.lock @@ -83,10 +83,6 @@ version "9.3.0" resolved "https://registry.yarnpkg.com/@types/node/-/node-9.3.0.tgz#3a129cda7c4e5df2409702626892cb4b96546dd5" -"@types/prop-types@^15.5.2": - version "15.5.2" - resolved "https://registry.yarnpkg.com/@types/prop-types/-/prop-types-15.5.2.tgz#3c6b8dceb2906cc87fe4358e809f9d20c8d59be1" - "@types/react-dom@^16.0.3": version "16.0.3" resolved "https://registry.yarnpkg.com/@types/react-dom/-/react-dom-16.0.3.tgz#8accad7eabdab4cca3e1a56f5ccb57de2da0ff64" From 85f23565208f7fb30b1bb54905348adbd1fcfaca Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Wed, 31 Jan 2018 11:48:19 -0800 Subject: [PATCH 11/16] remove submenu props docs --- packages/core/src/components/menu/menu.md | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/core/src/components/menu/menu.md b/packages/core/src/components/menu/menu.md index e07d9ebf03..50720c80ec 100644 --- a/packages/core/src/components/menu/menu.md +++ b/packages/core/src/components/menu/menu.md @@ -83,11 +83,6 @@ To add a submenu to a `Menu`, simply nest `MenuItem`s within another `MenuItem`. The submenu opens to the right of its parent by default, but will adjust and flip to the left if there is not enough room to the right. -`Menu` provides two `submenu` props to adjust this flipping behavior: you can customize the boundary element -and the padding within that boundary element. Note that a `MenuItem` _must be inside_ a `Menu` element -for these props to affect the submenus. On standalone `MenuItem`s (without a parent `Menu`, an anti-pattern) you can -use `popoverProps.modifiers` directly to achieve the same result. - ```jsx From 12958eec1e37bf5612c9e268cbf7a1a931e6352a Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Wed, 31 Jan 2018 11:54:01 -0800 Subject: [PATCH 12/16] props docs --- packages/core/src/components/menu/menuItem.tsx | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/core/src/components/menu/menuItem.tsx b/packages/core/src/components/menu/menuItem.tsx index 33f3f366a9..8efe37cad2 100644 --- a/packages/core/src/components/menu/menuItem.tsx +++ b/packages/core/src/components/menu/menuItem.tsx @@ -25,12 +25,11 @@ export interface IMenuItemProps extends IActionProps, ILinkProps { */ label?: string | JSX.Element; - /** Props to spread to `Popover`. The following props cannot be changed: `content`, `minimal`, `modifiers`. */ + /** Props to spread to `Popover`. Note that `content` and `minimal` cannot be changed. */ popoverProps?: Partial; /** - * Whether an enabled, non-submenu item should automatically close the - * popover it is nested within when clicked. + * Whether an enabled item without a submenu should automatically close its parent popover when clicked. * @default true */ shouldDismissPopover?: boolean; From 774173e64ea674457197bf2c8b7843200d4c510d Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Wed, 31 Jan 2018 12:48:59 -0800 Subject: [PATCH 13/16] delete menu test --- packages/core/test/menu/menuTests.tsx | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/packages/core/test/menu/menuTests.tsx b/packages/core/test/menu/menuTests.tsx index 9ed65c1aff..16d6515750 100644 --- a/packages/core/test/menu/menuTests.tsx +++ b/packages/core/test/menu/menuTests.tsx @@ -158,21 +158,6 @@ describe("Menu", () => { assert.isTrue(menu.hasClass(Classes.MENU)); assert.lengthOf(menu.find(MenuItem), 1); }); - - it("boundary props are exposed to children via context method", () => { - const modifiers = { boundariesElement: "window" as "window", padding: 100 }; - const menu = mount( - - - - - - , - ); - const { flip, preventOverflow } = menu.find(Popover).prop("modifiers"); - assert.deepEqual(flip, modifiers); - assert.deepEqual(preventOverflow, modifiers); - }); }); function findSubmenu(wrapper: ShallowWrapper) { From 472361dec81c8e53f92be124d957131aecfe1cd2 Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Wed, 31 Jan 2018 15:05:51 -0800 Subject: [PATCH 14/16] inline submenu Popover, fix positioning relative to parent item --- packages/core/src/components/menu/_submenu.scss | 6 ++---- packages/core/src/components/menu/menuItem.tsx | 8 ++++++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/core/src/components/menu/_submenu.scss b/packages/core/src/components/menu/_submenu.scss index 9f6b9bef0f..97682c1a50 100644 --- a/packages/core/src/components/menu/_submenu.scss +++ b/packages/core/src/components/menu/_submenu.scss @@ -41,12 +41,10 @@ } } - // minor adjustments to popover position so top submenu item lines up with submenu trigger item + // horizontal padding leaves some space from parent menu item, and extends mouse zone .pt-popover { - position: absolute; - top: -$half-grid-size; box-shadow: none; - padding-left: $half-grid-size; + padding: 0 $half-grid-size; &.pt-align-left { right: 0; diff --git a/packages/core/src/components/menu/menuItem.tsx b/packages/core/src/components/menu/menuItem.tsx index 8efe37cad2..8dd61642e7 100644 --- a/packages/core/src/components/menu/menuItem.tsx +++ b/packages/core/src/components/menu/menuItem.tsx @@ -102,6 +102,7 @@ export class MenuItem extends AbstractPureComponent { interactionKind={PopoverInteractionKind.HOVER} modifiers={SUBMENU_POPOVER_MODIFIERS} position={Position.RIGHT_TOP} + inline={true} {...popoverProps} content={{children}} minimal={true} @@ -128,6 +129,9 @@ export function renderMenuItem(props: IMenuItemProps, key: string | number) { } const SUBMENU_POPOVER_MODIFIERS: Popper.Modifiers = { - flip: { boundariesElement: "viewport", padding: 5 }, - preventOverflow: { boundariesElement: "viewport", padding: 5 }, + // 20px padding - scrollbar width + a bit + flip: { boundariesElement: "viewport", padding: 20 }, + // shift popover up 5px so MenuItems align + offset: { offset: -5 }, + preventOverflow: { boundariesElement: "viewport", padding: 20 }, }; From c9f43ca9126b18ae421de91658c4c8fb77a8d5ac Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Wed, 31 Jan 2018 15:05:57 -0800 Subject: [PATCH 15/16] button menu items inherit font --- packages/core/src/components/menu/_menu.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core/src/components/menu/_menu.scss b/packages/core/src/components/menu/_menu.scss index 9cf5287e59..63b056b7f6 100644 --- a/packages/core/src/components/menu/_menu.scss +++ b/packages/core/src/components/menu/_menu.scss @@ -116,6 +116,7 @@ button.pt-menu-item { background: none; width: 100%; text-align: left; + font-family: inherit; } .pt-menu-item-label { From cc9af72bf9fb0d6802d63fc7d26ec7d0ede61ace Mon Sep 17 00:00:00 2001 From: Gilad Gray Date: Wed, 31 Jan 2018 15:17:54 -0800 Subject: [PATCH 16/16] improve Overlay focus tests reliability --- packages/core/test/menu/menuTests.tsx | 2 -- packages/core/test/overlay/overlayTests.tsx | 27 +++++++++++---------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/packages/core/test/menu/menuTests.tsx b/packages/core/test/menu/menuTests.tsx index 16d6515750..ff4b55f2f5 100644 --- a/packages/core/test/menu/menuTests.tsx +++ b/packages/core/test/menu/menuTests.tsx @@ -111,7 +111,6 @@ describe("MenuItem", () => { // Ensures that popover props are passed to Popover component, except content property const popoverProps = { content: "CUSTOM_CONTENT", - inline: false, interactionKind: PopoverInteractionKind.CLICK, popoverClassName: "CUSTOM_POPOVER_CLASS_NAME", }; @@ -121,7 +120,6 @@ describe("MenuItem", () => { , ); - assert.strictEqual(wrapper.find(Popover).prop("inline"), popoverProps.inline); assert.strictEqual(wrapper.find(Popover).prop("interactionKind"), popoverProps.interactionKind); assert.notStrictEqual( wrapper diff --git a/packages/core/test/overlay/overlayTests.tsx b/packages/core/test/overlay/overlayTests.tsx index 3d8e5b083e..28c0f2049e 100644 --- a/packages/core/test/overlay/overlayTests.tsx +++ b/packages/core/test/overlay/overlayTests.tsx @@ -221,7 +221,10 @@ describe("", () => { , ); - assertFocus(".pt-overlay-backdrop", done); + assertFocus(() => { + const backdrops = Array.from(document.querySelectorAll(".pt-overlay-backdrop")); + assert.include(backdrops, document.activeElement); + }, done); }); it("does not bring focus to overlay if autoFocus=false", done => { @@ -273,7 +276,7 @@ describe("", () => { , ); wrapper.find(BACKDROP_SELECTOR).simulate("mousedown"); - assertFocus(`.${Classes.OVERLAY_CONTENT}`, done); + assertFocus(`h1.${Classes.OVERLAY_CONTENT}`, done); }); it("does not result in maximum call stack if two overlays open with enforceFocus=true", () => { @@ -345,18 +348,16 @@ describe("", () => { function assertFocus(selector: string | (() => void), done: MochaDone) { // the behavior being tested relies on requestAnimationFrame. - // use nested setTimeouts to delay till end of next frame. + // setTimeout for a few frames later to let things settle (to reduce flakes). setTimeout(() => { - setTimeout(() => { - wrapper.update(); - if (Utils.isFunction(selector)) { - selector(); - } else { - assert.strictEqual(document.querySelector(selector), document.activeElement); - } - done(); - }); - }); + wrapper.update(); + if (Utils.isFunction(selector)) { + selector(); + } else { + assert.strictEqual(document.querySelector(selector), document.activeElement); + } + done(); + }, 40); } });