From 8f8136d6fb9417e92ed08a223d50e2e1e009da11 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Tue, 17 Jan 2023 18:22:11 -0500 Subject: [PATCH] [core] fix dialog layout regressions, use DialogBody more (#5852) --- packages/core/src/common/classes.ts | 2 +- .../src/components/dialog/_dialog-body.scss | 8 +- .../src/components/dialog/_dialog-footer.scss | 2 +- packages/core/src/components/dialog/dialog.md | 17 ++--- .../src/components/dialog/dialogFooter.tsx | 12 ++- .../src/components/hotkeys/hotkeysDialog.tsx | 4 +- .../src/components/hotkeys/hotkeysDialog2.tsx | 5 +- packages/core/test/dialog/dialogTests.tsx | 76 ++++++++++--------- .../demo-app/src/examples/DialogExample.tsx | 6 +- .../examples/core-examples/drawerExample.tsx | 1 + .../core-examples/multistepDialogExample.tsx | 31 ++++---- 11 files changed, 83 insertions(+), 81 deletions(-) diff --git a/packages/core/src/common/classes.ts b/packages/core/src/common/classes.ts index 1f5fe1826a..e46727834b 100644 --- a/packages/core/src/common/classes.ts +++ b/packages/core/src/common/classes.ts @@ -122,7 +122,7 @@ export const DIALOG_BODY_SCROLL_CONTAINER = `${DIALOG}-body-scroll-container`; export const DIALOG_CLOSE_BUTTON = `${DIALOG}-close-button`; export const DIALOG_FOOTER = `${DIALOG}-footer`; export const DIALOG_FOOTER_FIXED = `${DIALOG}-footer-fixed`; -export const DIALOG_FOOTER_LEFT_SECTION = `${DIALOG}-footer-left-section`; +export const DIALOG_FOOTER_MAIN_SECTION = `${DIALOG}-footer-main-section`; export const DIALOG_FOOTER_ACTIONS = `${DIALOG}-footer-actions`; export const DIALOG_STEP = `${NS}-dialog-step`; diff --git a/packages/core/src/components/dialog/_dialog-body.scss b/packages/core/src/components/dialog/_dialog-body.scss index 419ebdc8ec..8fff971000 100644 --- a/packages/core/src/components/dialog/_dialog-body.scss +++ b/packages/core/src/components/dialog/_dialog-body.scss @@ -3,10 +3,16 @@ .#{$ns}-dialog-body { flex: 1 1 auto; - padding: $dialog-padding; + // We'd like to use padding instead of margin here to be consistent with the -dialog-body-scroll-container class, + // but we need to keep this margin style for backwards-compatibility. This may change in a future major version. + // TODO(adahiya): migrate from margin to padding style (CSS breaking change) + margin: $dialog-padding; } +// modifier for -dialog-body class, works similarly to -overlay-scroll-container .#{$ns}-dialog-body-scroll-container { + margin: 0; max-height: 70vh; overflow: auto; + padding: $dialog-padding; } diff --git a/packages/core/src/components/dialog/_dialog-footer.scss b/packages/core/src/components/dialog/_dialog-footer.scss index d2772e72d8..2ada7709f9 100644 --- a/packages/core/src/components/dialog/_dialog-footer.scss +++ b/packages/core/src/components/dialog/_dialog-footer.scss @@ -22,7 +22,7 @@ } } -.#{$ns}-dialog-footer-left-section { +.#{$ns}-dialog-footer-main-section { flex: 1 0 auto; } diff --git a/packages/core/src/components/dialog/dialog.md b/packages/core/src/components/dialog/dialog.md index 7b0b6792d0..53e8bd8fde 100644 --- a/packages/core/src/components/dialog/dialog.md +++ b/packages/core/src/components/dialog/dialog.md @@ -74,22 +74,19 @@ More examples of dialog content are shown below. @### Multistep dialog props -`MultistepDialog` is a wrapper around `Dialog` that displays a dialog with multiple steps, each of which maps to a specific panel. +`MultistepDialog` is a wrapper around `Dialog` that displays a dialog with multiple steps +Each step has a corresponding panel. -The children you provide to this component are rendered as contents inside the -`Classes.DIALOG` element. Typically, you will want to render a panel with -`Classes.DIALOG_BODY` that contains the body content for each step. - -Children of the `MultistepDialog` are filtered down to only `DialogStep` components and rendered in order. -`DialogStep` children are managed by the component; clicking one will change selection. +This component expects `DialogStep` children: each "step" is rendered in order +and its panel is shown as the dialog body content when the corresponding step is selected +in the navigation panel. @interface IMultistepDialogProps @### DialogStep `DialogStep` is a minimal wrapper with no functionality of its own—it is managed entirely by its -parent `MultistepDialog` wrapper. `DialogStep` title text can be set via the `title` prop. - -The associated step panel will be visible when the `DialogStep` is selected. +parent `MultistepDialog` wrapper. Typically, you should render a `` element as the `panel` +element. A step's title text can be set via the `title` prop. @interface IDialogStepProps diff --git a/packages/core/src/components/dialog/dialogFooter.tsx b/packages/core/src/components/dialog/dialogFooter.tsx index 50baa8eb86..36650161f8 100644 --- a/packages/core/src/components/dialog/dialogFooter.tsx +++ b/packages/core/src/components/dialog/dialogFooter.tsx @@ -65,20 +65,18 @@ export class DialogFooter extends AbstractPureComponent2 { })} role="dialogfooter" > - {this.maybeRenderLeftSection()} + {this.renderMainSection()} {this.maybeRenderActionsSection()} ); } - private maybeRenderLeftSection() { - const { children } = this.props; - if (children == null) { - return undefined; - } - return
{children}
; + /** Render the main footer section (left aligned). */ + private renderMainSection() { + return
{this.props.children}
; } + /** Optionally render the footer actions (right aligned). */ private maybeRenderActionsSection() { const { actions } = this.props; if (actions == null) { diff --git a/packages/core/src/components/hotkeys/hotkeysDialog.tsx b/packages/core/src/components/hotkeys/hotkeysDialog.tsx index d1074c3d4c..f0a84e9c5d 100644 --- a/packages/core/src/components/hotkeys/hotkeysDialog.tsx +++ b/packages/core/src/components/hotkeys/hotkeysDialog.tsx @@ -19,7 +19,7 @@ import * as React from "react"; import * as ReactDOM from "react-dom"; import { Classes } from "../../common"; -import { Dialog, DialogProps } from "../../components"; +import { Dialog, DialogBody, DialogProps } from "../../components"; import { Hotkey, IHotkeyProps } from "./hotkey"; import { Hotkeys } from "./hotkeys"; @@ -119,7 +119,7 @@ class HotkeysDialog { isOpen={this.isDialogShowing} onClose={this.hide} > -
{this.renderHotkeys()}
+ {this.renderHotkeys()} ); } diff --git a/packages/core/src/components/hotkeys/hotkeysDialog2.tsx b/packages/core/src/components/hotkeys/hotkeysDialog2.tsx index 51b1664246..d9170de8ba 100644 --- a/packages/core/src/components/hotkeys/hotkeysDialog2.tsx +++ b/packages/core/src/components/hotkeys/hotkeysDialog2.tsx @@ -20,6 +20,7 @@ import * as React from "react"; import { Classes } from "../../common"; import { HotkeyConfig } from "../../hooks"; import { Dialog, DialogProps } from "../dialog/dialog"; +import { DialogBody } from "../dialog/dialogBody"; import { Hotkey } from "./hotkey"; import { Hotkeys } from "./hotkeys"; @@ -36,7 +37,7 @@ export interface HotkeysDialog2Props extends DialogProps { export const HotkeysDialog2: React.FC = ({ globalGroupName = "Global", hotkeys, ...props }) => { return ( -
+ {hotkeys.map((hotkey, index) => ( = ({ globalGroupName /> ))} -
+ ); }; diff --git a/packages/core/test/dialog/dialogTests.tsx b/packages/core/test/dialog/dialogTests.tsx index eeccee55aa..cbc32caebd 100644 --- a/packages/core/test/dialog/dialogTests.tsx +++ b/packages/core/test/dialog/dialogTests.tsx @@ -19,15 +19,18 @@ import { mount } from "enzyme"; import * as React from "react"; import { spy } from "sinon"; -import { Button, Classes, Dialog, DialogProps, H4, Icon, IconSize } from "../../src"; +import { Button, Classes, Dialog, DialogBody, DialogFooter, DialogProps } from "../../src"; + +const COMMON_PROPS: Partial = { + icon: "inbox", + isOpen: true, + title: "Dialog header", + usePortal: false, +}; describe("", () => { it("renders its content correctly", () => { - const dialog = mount( - - {createDialogContents()} - , - ); + const dialog = mount({renderDialogBodyAndFooter()}); [ Classes.DIALOG, Classes.DIALOG_BODY, @@ -43,8 +46,8 @@ describe("", () => { it("portalClassName appears on Portal", () => { const TEST_CLASS = "test-class"; const dialog = mount( - - {createDialogContents()} + + {renderDialogBodyAndFooter()} , ); assert.isDefined(document.querySelector(`.${Classes.PORTAL}.${TEST_CLASS}`)); @@ -55,8 +58,8 @@ describe("", () => { const container = document.createElement("div"); document.body.appendChild(container); mount( - - {createDialogContents()} + + {renderDialogBodyAndFooter()} , ); assert.lengthOf(container.getElementsByClassName(Classes.DIALOG), 1, `missing ${Classes.DIALOG}`); @@ -66,8 +69,8 @@ describe("", () => { it("attempts to close when overlay backdrop element is moused down", () => { const onClose = spy(); const dialog = mount( - - {createDialogContents()} + + {renderDialogBodyAndFooter()} , ); dialog.find(`.${Classes.OVERLAY_BACKDROP}`).simulate("mousedown"); @@ -77,8 +80,8 @@ describe("", () => { it("doesn't close when canOutsideClickClose=false and overlay backdrop element is moused down", () => { const onClose = spy(); const dialog = mount( - - {createDialogContents()} + + {renderDialogBodyAndFooter()} , ); dialog.find(`.${Classes.OVERLAY_BACKDROP}`).simulate("mousedown"); @@ -88,8 +91,8 @@ describe("", () => { it("doesn't close when canEscapeKeyClose=false and escape key is pressed", () => { const onClose = spy(); const dialog = mount( - - {createDialogContents()} + + {renderDialogBodyAndFooter()} , ); dialog.simulate("keydown", { key: "Escape" }); @@ -99,7 +102,7 @@ describe("", () => { it("supports overlay lifecycle props", () => { const onOpening = spy(); mount( - + body , ); @@ -109,7 +112,7 @@ describe("", () => { describe("header", () => { it(`renders .${Classes.DIALOG_HEADER} if title prop is given`, () => { const dialog = mount( - + dialog body , ); @@ -118,7 +121,7 @@ describe("", () => { it(`renders close button if isCloseButtonShown={true}`, () => { const dialog = mount( - + dialog body , ); @@ -131,7 +134,7 @@ describe("", () => { it("clicking close button triggers onClose", () => { const onClose = spy(); const dialog = mount( - + dialog body , ); @@ -141,15 +144,15 @@ describe("", () => { }); it("only adds its className in one location", () => { - const dialog = mount(); + const dialog = mount(); assert.lengthOf(dialog.find(".foo").hostNodes(), 1); }); describe("accessibility features", () => { const mountDialog = (props: Partial) => { return mount( - - {createDialogContents()} + + {renderDialogBodyAndFooter()} , ); }; @@ -177,7 +180,7 @@ describe("", () => { }); it("does not apply default aria-labelledby if no title", () => { - const dialog = mountDialog({ className: "no-default-if-no-title" }); + const dialog = mountDialog({ className: "no-default-if-no-title", title: null }); // test existence here because id is generated assert.notExists(dialog.find(".no-default-if-no-title").hostNodes().prop("aria-labelledby")); }); @@ -196,25 +199,24 @@ describe("", () => { // N.B. everything else about Dialog is tested by Overlay - function createDialogContents(): JSX.Element[] { + function renderDialogBodyAndFooter(): JSX.Element[] { return [ -
- -

Dialog header

-
, -
+

Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod tempor incididunt ut labore et dolore magna alqua. Ut enim ad minimum veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.

-
, -
-
-
-
, + , + + ); diff --git a/packages/docs-app/src/examples/core-examples/drawerExample.tsx b/packages/docs-app/src/examples/core-examples/drawerExample.tsx index e82431e883..5f0eea5003 100644 --- a/packages/docs-app/src/examples/core-examples/drawerExample.tsx +++ b/packages/docs-app/src/examples/core-examples/drawerExample.tsx @@ -95,6 +95,7 @@ export class DrawerExample extends React.PureComponent
+ {/* HACKHACK: strange use of unrelated dialog class, should be refactored */}

diff --git a/packages/docs-app/src/examples/core-examples/multistepDialogExample.tsx b/packages/docs-app/src/examples/core-examples/multistepDialogExample.tsx index c6960d75f9..7e6bcced18 100644 --- a/packages/docs-app/src/examples/core-examples/multistepDialogExample.tsx +++ b/packages/docs-app/src/examples/core-examples/multistepDialogExample.tsx @@ -14,14 +14,13 @@ * limitations under the License. */ -import classNames from "classnames"; import * as React from "react"; import { Button, ButtonProps, - Classes, Code, + DialogBody, DialogStep, H5, HTMLSelect, @@ -206,7 +205,7 @@ export interface ISelectPanelProps { } const SelectPanel: React.FC = props => ( -

+

Use this dialog to divide content into multiple sequential steps.

Select one of the options below in order to proceed to the next step:

@@ -214,23 +213,21 @@ const SelectPanel: React.FC = props => ( -
+ ); export interface IConfirmPanelProps { selectedValue: string; } -const ConfirmPanel: React.FC = props => { - return ( -
-

- You selected Option {props.selectedValue}. -

-

- To make changes, click the "Back" button or click on the "Select" step. Otherwise, click "Close" to - complete your selection. -

-
- ); -}; +const ConfirmPanel: React.FC = props => ( + +

+ You selected Option {props.selectedValue}. +

+

+ To make changes, click the "Back" button or click on the "Select" step. Otherwise, click "Close" to complete + your selection. +

+ +);