[core] fix dialog layout regressions, use DialogBody more (#5852)

palantir · Jan 17, 2023 · 8f8136d · 8f8136d · adidahiya · Jan 17, 2023
1 parent 2ba88b8
commit 8f8136d
Show file tree

Hide file tree

Showing 11 changed files with 83 additions and 81 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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&mdash;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 `<DialogBody>` 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
@@ -65,20 +65,18 @@ export class DialogFooter extends AbstractPureComponent2<DialogFooterProps> {
                 })}
                 role="dialogfooter"
             >
-                {this.maybeRenderLeftSection()}
+                {this.renderMainSection()}
                 {this.maybeRenderActionsSection()}
             </div>
         );
     }
 
-    private maybeRenderLeftSection() {
-        const { children } = this.props;
-        if (children == null) {
-            return undefined;
-        }
-        return <div className={Classes.DIALOG_FOOTER_LEFT_SECTION}>{children}</div>;
+    /** Render the main footer section (left aligned). */
+    private renderMainSection() {
+        return <div className={Classes.DIALOG_FOOTER_MAIN_SECTION}>{this.props.children}</div>;
     }
 
+    /** 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
@@ -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}
             >
-                <div className={Classes.DIALOG_BODY}>{this.renderHotkeys()}</div>
+                <DialogBody>{this.renderHotkeys()}</DialogBody>
             </Dialog>
         );
     }

diff --git 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<HotkeysDialog2Props> = ({ globalGroupName = "Global", hotkeys, ...props }) => {
     return (
         <Dialog {...props} className={classNames(Classes.HOTKEY_DIALOG, props.className)}>
-            <div className={Classes.DIALOG_BODY}>
+            <DialogBody>
                 <Hotkeys>
                     {hotkeys.map((hotkey, index) => (
                         <Hotkey
@@ -46,7 +47,7 @@ export const HotkeysDialog2: React.FC<HotkeysDialog2Props> = ({ globalGroupName
                         />
                     ))}
                 </Hotkeys>
-            </div>
+            </DialogBody>
         </Dialog>
     );
 };
diff --git 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<DialogProps> = {
+    icon: "inbox",
+    isOpen: true,
+    title: "Dialog header",
+    usePortal: false,
+};
 
 describe("<Dialog>", () => {
     it("renders its content correctly", () => {
-        const dialog = mount(
-            <Dialog isOpen={true} usePortal={false}>
-                {createDialogContents()}
-            </Dialog>,
-        );
+        const dialog = mount(<Dialog {...COMMON_PROPS}>{renderDialogBodyAndFooter()}</Dialog>);
         [
             Classes.DIALOG,
             Classes.DIALOG_BODY,
@@ -43,8 +46,8 @@ describe("<Dialog>", () => {
     it("portalClassName appears on Portal", () => {
         const TEST_CLASS = "test-class";
         const dialog = mount(
-            <Dialog isOpen={true} portalClassName={TEST_CLASS}>
-                {createDialogContents()}
+            <Dialog {...COMMON_PROPS} usePortal={true} portalClassName={TEST_CLASS}>
+                {renderDialogBodyAndFooter()}
             </Dialog>,
         );
         assert.isDefined(document.querySelector(`.${Classes.PORTAL}.${TEST_CLASS}`));
@@ -55,8 +58,8 @@ describe("<Dialog>", () => {
         const container = document.createElement("div");
         document.body.appendChild(container);
         mount(
-            <Dialog isOpen={true} portalContainer={container}>
-                {createDialogContents()}
+            <Dialog {...COMMON_PROPS} usePortal={true} portalContainer={container}>
+                {renderDialogBodyAndFooter()}
             </Dialog>,
         );
         assert.lengthOf(container.getElementsByClassName(Classes.DIALOG), 1, `missing ${Classes.DIALOG}`);
@@ -66,8 +69,8 @@ describe("<Dialog>", () => {
     it("attempts to close when overlay backdrop element is moused down", () => {
         const onClose = spy();
         const dialog = mount(
-            <Dialog isOpen={true} onClose={onClose} usePortal={false}>
-                {createDialogContents()}
+            <Dialog {...COMMON_PROPS} onClose={onClose}>
+                {renderDialogBodyAndFooter()}
             </Dialog>,
         );
         dialog.find(`.${Classes.OVERLAY_BACKDROP}`).simulate("mousedown");
@@ -77,8 +80,8 @@ describe("<Dialog>", () => {
     it("doesn't close when canOutsideClickClose=false and overlay backdrop element is moused down", () => {
         const onClose = spy();
         const dialog = mount(
-            <Dialog canOutsideClickClose={false} isOpen={true} onClose={onClose} usePortal={false}>
-                {createDialogContents()}
+            <Dialog {...COMMON_PROPS} canOutsideClickClose={false} onClose={onClose}>
+                {renderDialogBodyAndFooter()}
             </Dialog>,
         );
         dialog.find(`.${Classes.OVERLAY_BACKDROP}`).simulate("mousedown");
@@ -88,8 +91,8 @@ describe("<Dialog>", () => {
     it("doesn't close when canEscapeKeyClose=false and escape key is pressed", () => {
         const onClose = spy();
         const dialog = mount(
-            <Dialog canEscapeKeyClose={false} isOpen={true} onClose={onClose} usePortal={false}>
-                {createDialogContents()}
+            <Dialog {...COMMON_PROPS} canEscapeKeyClose={false} onClose={onClose}>
+                {renderDialogBodyAndFooter()}
             </Dialog>,
         );
         dialog.simulate("keydown", { key: "Escape" });
@@ -99,7 +102,7 @@ describe("<Dialog>", () => {
     it("supports overlay lifecycle props", () => {
         const onOpening = spy();
         mount(
-            <Dialog isOpen={true} onOpening={onOpening}>
+            <Dialog {...COMMON_PROPS} onOpening={onOpening}>
                 body
             </Dialog>,
         );
@@ -109,7 +112,7 @@ describe("<Dialog>", () => {
     describe("header", () => {
         it(`renders .${Classes.DIALOG_HEADER} if title prop is given`, () => {
             const dialog = mount(
-                <Dialog isOpen={true} title="Hello!" usePortal={false}>
+                <Dialog {...COMMON_PROPS} title="Hello!">
                     dialog body
                 </Dialog>,
             );
@@ -118,7 +121,7 @@ describe("<Dialog>", () => {
 
         it(`renders close button if isCloseButtonShown={true}`, () => {
             const dialog = mount(
-                <Dialog isCloseButtonShown={true} isOpen={true} title="Hello!" usePortal={false}>
+                <Dialog {...COMMON_PROPS} isCloseButtonShown={true}>
                     dialog body
                 </Dialog>,
             );
@@ -131,7 +134,7 @@ describe("<Dialog>", () => {
         it("clicking close button triggers onClose", () => {
             const onClose = spy();
             const dialog = mount(
-                <Dialog isCloseButtonShown={true} isOpen={true} onClose={onClose} title="Hello!" usePortal={false}>
+                <Dialog {...COMMON_PROPS} isCloseButtonShown={true} onClose={onClose}>
                     dialog body
                 </Dialog>,
             );
@@ -141,15 +144,15 @@ describe("<Dialog>", () => {
     });
 
     it("only adds its className in one location", () => {
-        const dialog = mount(<Dialog className="foo" isOpen={true} title="title" usePortal={false} />);
+        const dialog = mount(<Dialog {...COMMON_PROPS} className="foo" />);
         assert.lengthOf(dialog.find(".foo").hostNodes(), 1);
     });
 
     describe("accessibility features", () => {
         const mountDialog = (props: Partial<DialogProps>) => {
             return mount(
-                <Dialog isOpen={true} usePortal={false} {...props}>
-                    {createDialogContents()}
+                <Dialog {...COMMON_PROPS} {...props}>
+                    {renderDialogBodyAndFooter()}
                 </Dialog>,
             );
         };
@@ -177,7 +180,7 @@ describe("<Dialog>", () => {
         });
 
         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("<Dialog>", () => {
 
     // N.B. everything else about Dialog is tested by Overlay
 
-    function createDialogContents(): JSX.Element[] {
+    function renderDialogBodyAndFooter(): JSX.Element[] {
         return [
-            <div className={Classes.DIALOG_HEADER} key={0}>
-                <Icon icon="inbox" size={IconSize.LARGE} />
-                <H4 id="dialog-title">Dialog header</H4>
-            </div>,
-            <div className={Classes.DIALOG_BODY} key={1}>
+            <DialogBody key="body">
                 <p id="dialog-description">
                     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.
                 </p>
-            </div>,
-            <div className={Classes.DIALOG_FOOTER} key={2}>
-                <div className={Classes.DIALOG_FOOTER_ACTIONS}>
-                    <Button text="Secondary" />
-                    <Button className={Classes.INTENT_PRIMARY} type="submit" text="Primary" />
-                </div>
-            </div>,
+            </DialogBody>,
+            <DialogFooter
+                key="footer"
+                actions={
+                    <>
+                        <Button text="Secondary" />
+                        <Button className={Classes.INTENT_PRIMARY} type="submit" text="Primary" />
+                    </>
+                }
+            />,
         ];
     }
 });
diff --git a/packages/demo-app/src/examples/DialogExample.tsx b/packages/demo-app/src/examples/DialogExample.tsx
@@ -16,7 +16,7 @@
 
 import * as React from "react";
 
-import { Button, Classes, Dialog } from "@blueprintjs/core";
+import { Button, Dialog, DialogBody } from "@blueprintjs/core";
 
 import { ExampleCard } from "./ExampleCard";
 
@@ -44,11 +44,11 @@ export class DialogExample extends React.PureComponent<DialogExampleProps, Dialo
                     icon="info-sign"
                     title="Dialog header"
                 >
-                    <div className={Classes.DIALOG_BODY}>
+                    <DialogBody>
                         Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut
                         labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco
                         laboris nisi ut aliquip ex ea commodo consequat
-                    </div>
+                    </DialogBody>
                 </Dialog>
             </ExampleCard>
         );

diff --git 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<ExampleProps<IBlueprintEx
                     {...this.state}
                 >
                     <div className={Classes.DRAWER_BODY}>
+                        {/* HACKHACK: strange use of unrelated dialog class, should be refactored */}
                         <div className={Classes.DIALOG_BODY}>
                             <p>
                                 <strong>