[core] feat(Dialog): modernise visuals, body & footer components #5753

CPerinet · 2022-11-10T14:26:05Z

Checklist

Update documentation

Changes proposed in this pull request:

Reduced title from H5 to H6
Header min-height is now based on the button size + padding.
Use light-gray5 as default background colour for light mode
Enable scrolling of the body past a given height
Add a fixed footer. This is especially useful when the dialog body is likely to scroll.
- Made it use the same class for both normal and multi-step dialog
- Kept current footer classes

Screenshot

adidahiya · 2022-11-14T15:49:39Z

packages/core/src/components/dialog/dialog.tsx

@@ -121,14 +126,19 @@ export class Dialog extends AbstractPureComponent2<DialogProps> {
            <Overlay {...this.props} className={Classes.OVERLAY_SCROLL_CONTAINER} hasBackdrop={true}>
                <div className={Classes.DIALOG_CONTAINER} ref={this.props.containerRef}>
                    <div
-                        className={classNames(Classes.DIALOG, this.props.className)}
+                        className={classNames(Classes.DIALOG, this.props.footer != null && Classes.DIALOG_NO_PADDING, this.props.className)}


classNames has a slightly nicer syntax for conditional classes:

Suggested change

className={classNames(Classes.DIALOG, this.props.footer != null && Classes.DIALOG_NO_PADDING, this.props.className)}

className={classNames(Classes.DIALOG, this.props.className, {

[Classes.DIALOG_NO_PADDING]: this.props.footer != null,

})}

adidahiya · 2022-11-14T15:58:55Z

packages/core/src/common/classes.ts

@@ -197,7 +200,6 @@ export const MULTISTEP_DIALOG = `${NS}-multistep-dialog`;
 export const MULTISTEP_DIALOG_PANELS = `${MULTISTEP_DIALOG}-panels`;
 export const MULTISTEP_DIALOG_LEFT_PANEL = `${MULTISTEP_DIALOG}-left-panel`;
 export const MULTISTEP_DIALOG_RIGHT_PANEL = `${MULTISTEP_DIALOG}-right-panel`;
-export const MULTISTEP_DIALOG_FOOTER = `${MULTISTEP_DIALOG}-footer`;


this is part of the public API, so unfortunately we can't remove it so easily in this PR. we'll have to keep this constant here and continue adding the class name to the DOM in packages/core/src/components/dialog/multistepDialog.tsx on L220 (this will also fix the failing tests in this PR), but we can remove its Sass styles and rely on a new class name for the updated styling.

adidahiya · 2022-11-14T16:01:28Z

packages/core/src/components/dialog/dialog.tsx

+                        <div className={Classes.DIALOG_BODY_SCROLL_CONTAINER}>
+                            {this.props.children}
+                        </div>


I'm afraid this may be a breaking change for some consumers who are applying their own scroll/overflow styles to dialogs. We will have to add it in a non-breaking way with a new prop, something like useScrollableBodyContainer?: boolean where the default is false to preserve existing behavior.

Or: see my comment below with an API proposal with 3 new components.

adidahiya · 2022-11-14T16:19:17Z

packages/core/src/components/dialog/dialog.tsx

+                            {this.props.children}
+                        </div>
+
+                        {this.maybeRenderFixedFooter()}


The ergonomics / developer experience of this new footer feel a little awkward to me right now.

We've kept the Dialog component pretty simple so far, but if we're trying to add more functionality to improve & evolve its layout, we may want to reconsider the design choice of using only a single React component.

I think if we add 3 new components, we can create an API that's more legible while still preserving backwards-compatibility:

<Dialog isOpen={...} onClose={...}> <DialogHeader title="..." icon="..." isCloseButtonShown={...} /> <DialogBody useOverflowScrollContainer={...}> // <-- scroll container would be enabled by default {/* children go here */} </DialogBody> <DialogFooter fixed={...} actions={...}> {/* footer left content goes here */} </DialogFooter> </Dialog>

If you like this direction, I can help you implement these changes in this PR.

I don't have a strong preference here. I've also seen the following pattern:

<Dialog header={IDialogHeader.basic( { title: "Title", icon: IconNames.FOLDER, isCloseButtonShown: false, }) footer={IDialogFooter.fixed( { left: "", actions: "" }) useOverflowScrollContainer={true} > // Content </Dialog>

That may help with discoverability of features? Not sure how back-compatible that would be.

(I'm going to hold off on making the other changes while we discuss the API)

CPerinet · 2023-01-09T19:21:29Z

packages/core/src/common/classes.ts

@@ -116,8 +116,14 @@ export const CONTROL_GROUP = `${NS}-control-group`;
 export const DIALOG = `${NS}-dialog`;
 export const DIALOG_CONTAINER = `${DIALOG}-container`;
 export const DIALOG_BODY = `${DIALOG}-body`;
+export const DIALOG_BODY_NO_PADDING = `${DIALOG}-body-no-padding`;


not only provides padding but also scroll. Thus, I'm adding a way to remove padding as just not using is not practical.
Adding a "subtractive" class to keep DIALOG_BODY as close from current.

it seems like this class is not being demonstrated in the example... do we need to add it to the public API right now? maybe we can hold off on adding it until we find a concrete use case for it. until then, users can apply custom styles to get padding: 0.

CPerinet · 2023-01-09T19:21:59Z

packages/core/src/common/classes.ts

@@ -116,8 +116,14 @@ export const CONTROL_GROUP = `${NS}-control-group`;
 export const DIALOG = `${NS}-dialog`;
 export const DIALOG_CONTAINER = `${DIALOG}-container`;
 export const DIALOG_BODY = `${DIALOG}-body`;
+export const DIALOG_BODY_NO_PADDING = `${DIALOG}-body-no-padding`;
+export const DIALOG_BODY_SCROLL_CONTAINER = `${DIALOG}-body-scroll-container`;


Class applied when making scrollable.

CPerinet · 2023-01-09T19:22:59Z

packages/core/src/common/classes.ts

 export const DIALOG_CLOSE_BUTTON = `${DIALOG}-close-button`;
 export const DIALOG_FOOTER = `${DIALOG}-footer`;
+export const DIALOG_FOOTER_CONTAINER = `${DIALOG}-footer-container`;


Needed to apply flex properties on left and right side of the footer. Did not directly added to DIALOG_FOOTER to keep it as current.

Hmm, I don't think this is quite working like we want it to. I think we can do this without an additional level of DOM nesting, by using a modifier class instead. I'll take a stab at that approach.

before after

CPerinet · 2023-01-09T19:23:25Z

packages/core/src/common/classes.ts

 export const DIALOG_CLOSE_BUTTON = `${DIALOG}-close-button`;
 export const DIALOG_FOOTER = `${DIALOG}-footer`;
+export const DIALOG_FOOTER_CONTAINER = `${DIALOG}-footer-container`;
+export const DIALOG_FIXED_FOOTER = `${DIALOG}-fixed-footer`;


Styling for the fixed footer (white background, border &c.)

CPerinet · 2023-01-09T19:23:46Z

packages/core/src/common/classes.ts

 export const DIALOG_CLOSE_BUTTON = `${DIALOG}-close-button`;
 export const DIALOG_FOOTER = `${DIALOG}-footer`;
+export const DIALOG_FOOTER_CONTAINER = `${DIALOG}-footer-container`;
+export const DIALOG_FIXED_FOOTER = `${DIALOG}-fixed-footer`;
+export const DIALOG_INLINE_FOOTER = `${DIALOG}-inline-footer`;


Same for inline, in this case, it is just spacing.

I think a better name for this would be "minimal", as it's a kind of minimal appearance. "Inline" is a bit confusing because it's not using inline layout; it's a "block" element with display: flex.

CPerinet · 2023-01-09T19:24:59Z

packages/core/src/components/dialog/_dialog-body.scss

@@ -0,0 +1,13 @@
+.#{$ns}-dialog-body {
+  flex: 1 1 auto;
+  padding: $dialog-padding;


Changed margin to padding so the scroll works better. Should not cause regressions.
Removed line-height, did not seem appropriate default.

CPerinet · 2023-01-09T19:25:46Z

packages/core/src/components/dialog/_dialog-footer.scss

+  flex: 1 0 auto;
+}
+
+.#{$ns}-dialog-footer-actions {


Moved, no changes

CPerinet · 2023-01-09T19:27:36Z

packages/core/src/components/dialog/_dialog-footer.scss

@@ -0,0 +1,40 @@
+.#{$ns}-dialog-footer {
+  flex: 0 0 auto;


Removed side margin. Will break things. I can add a class to keep it and then suppress it if using the new component.

Yeah, we'll need to keep the existing margin and add any new functionality with a modifier class.

adidahiya

I'd like to push a commit to address my comments about the breaking changes. I also want to clean up a few things, fix lint, etc.

adidahiya · 2023-01-09T22:56:43Z

packages/core/src/components/dialog/_dialog-footer.scss

@@ -0,0 +1,40 @@
+.#{$ns}-dialog-footer {
+  flex: 0 0 auto;


Yeah, we'll need to keep the existing margin and add any new functionality with a modifier class.

adidahiya · 2023-01-09T22:58:02Z

packages/core/src/components/dialog/_dialog.scss

-
-.#{$ns}-dialog-body {
-  flex: 1 1 auto;
-  line-height: $pt-grid-size * 1.8;


unfortunately we need to keep this style too, it's too risky to change, and it's part of our API contract not to break styles on public class names like this.

actually, I take this back -- this is the default line height, and it's applied by the base-typography() Sass mixin on body already, so it's probably ok to remove this style 👍

adidahiya · 2023-01-09T23:28:22Z

packages/core/src/components/dialog/_dialog-footer.scss

+  border-radius: 0  0 $dialog-border-radius $dialog-border-radius;
+  border-top: 1px solid $pt-divider-black;
+  padding: $pt-grid-size;
+  padding-left: $dialog-padding;


why the extra padding here? is this intentional? if so, should we add the extra (5px) padding on the right side as well?

before after

I see now that it's a little tricky to design this style to work for both button and non-button content. In the docs example we have some text in the "left" section for Dialog, but there's a button in that same place for MultiStepDialog...

original CR was addressed, now iterating on final API choices & design

…ents

adidahiya · 2023-01-10T14:33:02Z

There's a bug in the preview script which is now failing on builds from forks. I'll fix that separately. For now you can view the latest docs preview here: https://output.circle-artifacts.com/output/job/a9d924bc-9ef0-4993-8939-2eb3fcabbd68/artifacts/0/packages/docs-app/dist/index.html

Update dialog

b3df271

CPerinet requested a review from adidahiya November 10, 2022 14:26

Charles Perinet added 2 commits November 10, 2022 14:36

Update multi-step dialog

5f64217

lint

da79ea1

CPerinet marked this pull request as ready for review November 10, 2022 14:51

adidahiya previously requested changes Nov 14, 2022

View reviewed changes

Charles Perinet added 3 commits January 5, 2023 17:20

Add DialogBody && DialogFooter

29cd7bc

Remove no padding class

f531010

Reformat

d530544

CPerinet commented Jan 9, 2023

View reviewed changes

Cleanup example

4e40eb0

adidahiya reviewed Jan 9, 2023

View reviewed changes

Refactor to preserve existing styles, fix lint

d892eda

CPerinet requested a review from adidahiya January 10, 2023 10:17

CPerinet mentioned this pull request Jan 10, 2023

[core] feat(Callout): visual modernisation #5834

Merged

adidahiya added 3 commits January 10, 2023 08:53

Deprecate MSD class, fix tests

e25c1ff

Merge remote-tracking branch 'origin/develop' into cp/dialog-improvem…

e8f2718

…ents

add/improve docs

b5f6c19

adidahiya approved these changes Jan 10, 2023

View reviewed changes

adidahiya changed the title ~~Update dialog~~ [core] feat(Dialog): modernise visuals, add body & footer components Jan 10, 2023

disable body scroll container when footer is minimal

e7effa1

adidahiya changed the title ~~[core] feat(Dialog): modernise visuals, add body & footer components~~ [core] feat(Dialog): modernise visuals, body & footer components Jan 10, 2023

adidahiya merged commit a154e34 into palantir:develop Jan 10, 2023

adidahiya mentioned this pull request Jan 17, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] feat(Dialog): modernise visuals, body & footer components #5753

[core] feat(Dialog): modernise visuals, body & footer components #5753

CPerinet commented Nov 10, 2022 •

edited

Loading

adidahiya Nov 14, 2022

adidahiya Nov 14, 2022

adidahiya Nov 14, 2022

adidahiya Nov 14, 2022

CPerinet Nov 14, 2022 •

edited

Loading

CPerinet Jan 9, 2023

adidahiya Jan 9, 2023

CPerinet Jan 9, 2023

CPerinet Jan 9, 2023

adidahiya Jan 9, 2023 •

edited

Loading

CPerinet Jan 9, 2023

CPerinet Jan 9, 2023

adidahiya Jan 9, 2023

CPerinet Jan 9, 2023

CPerinet Jan 9, 2023

CPerinet Jan 9, 2023

adidahiya Jan 9, 2023

adidahiya left a comment

adidahiya Jan 9, 2023

adidahiya Jan 9, 2023

adidahiya Jan 10, 2023

adidahiya Jan 9, 2023

adidahiya Jan 9, 2023

adidahiya commented Jan 10, 2023

-                        className={classNames(Classes.DIALOG, this.props.footer != null && Classes.DIALOG_NO_PADDING, this.props.className)}
+                        className={classNames(Classes.DIALOG, this.props.className, {
+                            [Classes.DIALOG_NO_PADDING]: this.props.footer != null,
+                        })}

[core] feat(Dialog): modernise visuals, body & footer components #5753

[core] feat(Dialog): modernise visuals, body & footer components #5753

Conversation

CPerinet commented Nov 10, 2022 • edited Loading

Checklist

Changes proposed in this pull request:

Screenshot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPerinet Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya Jan 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya commented Jan 10, 2023

CPerinet commented Nov 10, 2022 •

edited

Loading

CPerinet Nov 14, 2022 •

edited

Loading

adidahiya Jan 9, 2023 •

edited

Loading