Add a multistep dialog component #4462

ycvfu · 2020-12-17T17:27:48Z

This PR adds a multistep dialog component.

Checklist

Includes tests
Update documentation

Changes proposed in this pull request:

This PR aims to create a new multistep dialog component that enables the users to move through a sequence of steps in a dialog, each of which presents a different panel. It is implemented as a wrapper around Dialog.

This PR is an updated version of #4455 from a forked repository. I am temporarily keeping #4455 open as other contributors may be looking at it (@kmblake are you planning on updating the old PR/referencing the old PR, or is it okay for me to close it?).

Reviewers should focus on:

The dialog should present to the user a summary of steps on the left and the main panel associated with the currently selected panel on the right. When the user is on the first step, there should only be a 'next' or 'submit' footer button as appropriate. When the user is on the last step, there should only be a 'submit' footer button and potentially a 'back' footer button. When the user is on any page in between, they should see a back and next button.

The user also has the option of navigating between viewed steps on the left panel by clicking on a step. Only steps that have been previously accessed via clicking the "next" footer button can be clicked on. The user can select any pre-existing steps to see the panel.

All regular functionalities from Dialog should also be present.

Screenshot

…ck for length of steps, etc.

kmblake

Thanks for contributing! I've provided some initial comments but ultimately defer to @adidahiya. Also @ycvfu if you want to offload this PR just let me know--I'm happy to own any additional changes to get it over the line after the holidays.

kmblake · 2020-12-18T09:36:13Z

packages/core/src/components/dialog/dialog.md

+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.


I think we should explain a little more about the Step interface similar to the docs for Tab. Also probably worth clarifying that only children of type Step will be rendered.

Sounds good, I added info under line 54.

kmblake · 2020-12-18T09:43:01Z

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

+    /**
+     * Props for the button to display on the final step. To illustrate, this may be a submit button.
+     */
+    finalButtonProps?: IFinalButtonProps;


I know there was some earlier discussion about not using Partial<IButtonProps> here because we wanted to prevent users from changing things like button size, but it's a bit difficult to interpret what these IFinalButtonProps actually includes based on the type declaration. I would be in favor of Partial<IButtonProps> or at least adding some more clarity in the comment.

kmblake · 2020-12-18T09:48:46Z

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

+}
+
+function isNextButtonEnabled(step: StepElement) {
+    return step.props.nextButtonEnabledByDefault !== undefined ? step.props.nextButtonEnabledByDefault : true;


Suggested change

return step.props.nextButtonEnabledByDefault !== undefined ? step.props.nextButtonEnabledByDefault : true;

return step.props.nextButtonEnabledByDefault ?? true;

kmblake · 2020-12-18T09:58:00Z

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

+    private renderLeftPanel() {
+        return (
+            <div className={Classes.MULTISTEP_DIALOG_LEFT_PANEL}>
+                {React.Children.toArray(this.props.children).filter(isStepElement).map(this.renderStep)}


Suggested change

{React.Children.toArray(this.props.children).filter(isStepElement).map(this.renderStep)}

{this.getStepChildren().map(this.renderStep)}

kmblake · 2020-12-18T10:01:46Z

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

+        const hasBeenViewed = this.state.lastViewedIndex >= index;
+        const currentlySelected = this.state.selectedIndex === index;
+        return (
+            <div className={classNames(Classes.STEP_CONTAINER, hasBeenViewed && Classes.ACTIVE)} key={index}>


Suggested change

<div className={classNames(Classes.STEP_CONTAINER, hasBeenViewed && Classes.ACTIVE)} key={index}>

<div className={classNames(Classes.STEP_CONTAINER, { [Classes.ACTIVE]: hasBeenViewed })} key={index}>

Looks like other places in BP use this syntax for conditional classes but defer to @adidahiya

kmblake · 2020-12-18T10:46:27Z

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

+        if (this.props.finalButtonProps !== undefined) {
+            return <Button key="final" {...this.props.finalButtonProps} />;
+        } else {
+            return <Button intent="danger" key="final" onClick={this.props.onClose} text="Submit" />;
+        }


Suggested change

if (this.props.finalButtonProps !== undefined) {

return <Button key="final" {...this.props.finalButtonProps} />;

} else {

return <Button intent="danger" key="final" onClick={this.props.onClose} text="Submit" />;

}

return <Button intent="primary" key="final" onClick={this.props.onClose} text="Submit" {...this.props.finalButtonProps} />

Similar to how buttonProps are handled in TimezonePicker

Also, don't think the submit button should default to intent "danger"

kmblake · 2020-12-18T11:00:52Z

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

+    /**
+     * Indicates whether the next button should be enabled by default on this step
+     */
+    nextButtonEnabledByDefault?: boolean;


Wondering if we should just expose nextButtonProps on MultistepDialog and make the next button fully controlled. Then we wouldn't need nextButtonEnabledByDefault or updateDialog, and Step would be a purely presentational component that could just render its children. If the next button needs to be disabled at the start of certain steps, the client would do that in their change handler for MultistepDialog's onChange. Since the parent component of MultistepDialog is ultimately responsible for keeping track of what data has been entered in the dialog, it might make sense to just allow it to control when the next button is enabled as well.

Hmm, maybe we can expose a disableNext prop in the MultistepDialog instead of exposing nextButtonProps. I don't think that the user should be able to control any aspect of this button except its enabled state. If the user could change the button design such as the text, intent, etc. it would enable them to deviate from the Blueprint design, and I can't think of a case where this would be desirable (since we want to standardize this). For instance, enabling them to change a button on step 3 out of 5 to show "submit" would break the flow and cause confusion.

Sounds good, I think just exposing disableNext on MultistepDialog is sufficient! In that case, we should remove this prop from Step, right?

Yeah! My bad, I missed that. I fixed it in the latest revision.

packages/core/src/components/dialog/_multistep-dialog.scss

kmblake · 2020-12-18T11:35:19Z

packages/core/src/components/dialog/_multistep-dialog.scss

+}
+
+.#{$ns}-multistep-dialog-left-panel {
+  border-radius: 0 $dialog-border-radius 0 0;


Do we need this given that this div doesn't have a background color?

kmblake

Looking good, mostly just comments on completing the transition to make disableNext controlled at the dialog level rather than handled by each step.

kmblake · 2021-01-05T04:59:38Z

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

+    /**
+     * Indicates whether the next button should be enabled by default on this step
+     */
+    nextButtonEnabledByDefault?: boolean;


Sounds good, I think just exposing disableNext on MultistepDialog is sufficient! In that case, we should remove this prop from Step, right?

kmblake · 2021-01-05T05:00:37Z

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

+    }
+}
+
+function isNextButtonEnabled(step: StepElement) {


No longer needed if we're exposing the disableNext prop

kmblake · 2021-01-05T05:06:43Z

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

+        }
+    }
+
+    private getDefaultState() {


If we only need to define lastViewedIndex and selectedIndex, probably a little clearer to just define those in an INITIAL_STATE const and then set state to INITIAL_STATE in the constructor and componentDidUpdate

kmblake · 2021-01-05T05:15:43Z

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

+                    intent="primary"
+                    key="next"
+                    onClick={this.handleClickNext()}
+                    text="Next"


@adidahiya how does BP handle localization? It seems like for most components the client supplies all of the copy. Should that also be the case here (at least making it possible to override the button text)?

Yeah, I think we need to add a prop to override the button text for i18n. See my comment on IMultistepDialogProps.

kmblake · 2021-01-05T05:20:34Z

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

+
+export type StepId = string | number;
+
+export interface IMultistepDialogPanelProps {


No longer used?

…ample.tsx

adidahiya

PR preview build

Looking great so far! The docs example is currently pretty simple, but we can flesh it out more in a future PR.

It's a little unfortunate how the header icon is misaligned with the step icons here. If we're going to keep the padding this way, we should change the header icon in the example to something that's not a circle.

adidahiya · 2021-01-06T19:27:39Z

packages/core/src/components/dialog/_multistep-dialog.scss

+  align-items: center;
+  background-color: $light-gray5;
+  border-radius: $step-radius;
+  cursor: not-allowed;


I don't think this is strictly necessary, it can just be grayed out and have a normal pointer with no hover state. Also, what about previous steps? Can't you click on those?

On further review, I realized the semantics of "active" here are a little different from what I expected. That's fine, I think what you have works. but I think it would help to leave a code comment along the lines of "by default, steps are inactive until they are visited"

adidahiya · 2021-01-06T19:31:36Z

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

+
+    public constructor(props: IMultistepDialogProps) {
+        super(props);
+        this.state = INITIAL_STATE;


nit: instead of using a constructor, we can just write

public state: IMultistepDialogState = { lastViewedIndex: 0, selectedIndex: 0, };

adidahiya · 2021-01-06T19:35:06Z

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

+
+    public componentDidUpdate(prevProps: IMultistepDialogProps) {
+        if (!prevProps.isOpen && this.props.isOpen) {
+            this.setState(INITIAL_STATE);


Do we always want to reset to initial state when a dialog is re-opened? Let's document this design decision somewhere in the docs. Maybe this behavior should be controlled by a prop in the future.

Hmm, yeah I made the assumption here that if we closed a dialog or clicked away from it, we would want any progress would be lost. We can expose a prop like "resetOnClose" to enable the user to control this.

adidahiya · 2021-01-06T19:51:47Z

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

+
+    private handleClickStep = (index: number) => {
+        if (index > this.state.lastViewedIndex) {
+            return undefined;


nit: this code path doesn't need to return a value; I would either return; here or reverse the condition to if (index <= this.state.lastViewedIndex)

adidahiya · 2021-01-06T19:52:50Z

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

+                    intent="primary"
+                    key="next"
+                    onClick={this.handleClickNext()}
+                    text="Next"


Yeah, I think we need to add a prop to override the button text for i18n. See my comment on IMultistepDialogProps.

adidahiya · 2021-01-06T19:56:54Z

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

+    /**
+     * Whether to disable the next button in the panel footer.
+     */
+    disableNext?: boolean;


I propose making this a little more flexible and solving the problem of the button text mentioned below with a props object here.

nextButtonProps?: Partial<Pick<IButtonProps, "disabled" | "text">>;

adidahiya · 2021-01-06T19:57:30Z

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

+        };
+    }
+
+    private getFinalButton() {


nit: renderFinalButton() since this returns JSX

adidahiya · 2021-01-06T19:58:17Z

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

+
+    /**
+     * Panel content, rendered by the parent `MultistepDialog` when this step is active.
+     * If omitted, no panel will be rendered for this step.


what does this mean / look like? why wouldn't there be a panel?

adidahiya · 2021-01-06T20:00:34Z

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

+}
+
+@polyfill
+export class Step extends AbstractPureComponent2<IStepProps> {


I think we need to be more explicit about the naming of this component and its CSS classes. They should all be prefixed with "dialog" because they correspond to MultiStepDialog. This helps disambiguate from other potential "step" components, like those proposed in #4058 / #260.

adidahiya · 2021-01-06T20:03:52Z

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

+        return (
+            <div className={classNames(Classes.STEP_CONTAINER, { [Classes.ACTIVE]: hasBeenViewed })} key={index}>
+                <div
+                    className={classNames(Classes.STEP, { [Classes.ACTIVE]: hasBeenViewed })}


do we need all 4 active classes? I think we can remove this one and the one on L113 and change the Sass to be something like:

.step { .active & { ... } }

…n text, etc

ycvfu · 2021-01-07T23:50:09Z

packages/core/src/components/dialog/_multistep-dialog.scss

+  cursor: not-allowed;
+  display: flex;
+  margin: 4px;
+  padding: 6px 14px;


Changed padding to better align with the title icon.

…ment('div')

adidahiya · 2021-01-08T05:14:26Z

Latest changes look good! But it looks like we need to add dark theme support:

adidahiya

looks good! thanks for finishing this up @ycvfu 🎉

fycvanessa and others added 4 commits December 11, 2020 11:48

Add multistep dialog to bp

3c1a376

fixing tests and linting

aed6c90

Merge remote-tracking branch 'upstream/develop' into vf/multistep-dialog

3ad28bc

Adding tests and documentation

a5c2d05

ycvfu requested a review from kmblake December 17, 2020 17:28

Vanessa Fu added 5 commits December 17, 2020 13:14

Cleanup: changing ISubmitButtonProps to IFinalButtonProps, adding che…

368f935

…ck for length of steps, etc.

Uncomment steps from multistepDialogExample

017cb65

Re-run yarn format

53f6c3c

Remove unnecessary change to package.json

6e8def9

Fixing yarn dist issue

17c9744

kmblake reviewed Dec 18, 2020

View reviewed changes

Pr fixes (and react15 test fixes)

6187de0

ycvfu requested review from adidahiya and kmblake and removed request for kmblake January 4, 2021 15:58

kmblake reviewed Jan 5, 2021

View reviewed changes

Vanessa Fu added 2 commits January 5, 2021 20:52

Pr fixes: cleaning up methods relating to the enable next state

69fd270

Remove nextButtonEnabledByDefault prop from step in multistepDialogEx…

90d6c8a

…ample.tsx

ycvfu requested a review from kmblake January 6, 2021 02:49

adidahiya suggested changes Jan 6, 2021

View reviewed changes

Pr fixes: renaming step to dialogStep, adding override for next butto…

931f069

…n text, etc

ycvfu commented Jan 7, 2021

View reviewed changes

Vanessa Fu added 3 commits January 7, 2021 19:29

Cleanup

9027e98

cleaning up isotest

8104acf

Cleaning up isotest pt.2: adding back DialogStep with React.createEle…

29d0f1d

…ment('div')

ycvfu requested a review from adidahiya January 8, 2021 02:16

Pr fixes: adding dark theme support

9fb304d

adidahiya approved these changes Jan 9, 2021

View reviewed changes

adidahiya merged commit f4cf4a7 into palantir:develop Jan 9, 2021

	return step.props.nextButtonEnabledByDefault !== undefined ? step.props.nextButtonEnabledByDefault : true;
	return step.props.nextButtonEnabledByDefault ?? true;

	{React.Children.toArray(this.props.children).filter(isStepElement).map(this.renderStep)}
	{this.getStepChildren().map(this.renderStep)}

	<div className={classNames(Classes.STEP_CONTAINER, hasBeenViewed && Classes.ACTIVE)} key={index}>
	<div className={classNames(Classes.STEP_CONTAINER, { [Classes.ACTIVE]: hasBeenViewed })} key={index}>


		export type StepId = string \| number;

		export interface IMultistepDialogPanelProps {

Add a multistep dialog component #4462

Add a multistep dialog component #4462

Conversation

ycvfu commented Dec 17, 2020

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

kmblake left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycvfu Jan 4, 2021 • 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

ycvfu Dec 30, 2020 • 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

kmblake 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

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

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 commented Jan 8, 2021

adidahiya left a comment

Choose a reason for hiding this comment

ycvfu Jan 4, 2021 •

edited

Loading

ycvfu Dec 30, 2020 •

edited

Loading