[core] feat(MultistepDialog): navigationPosition prop #4970

mud · 2021-10-15T22:07:09Z

Follow up to #4969

This PR builds on top of #4969 by adding an option to position the navigation element in MultistepDialog. In addition to the default (Position.LEFT) Position.TOP and Position.RIGHT are possible.

Checklist

Includes tests
Update documentation

Changes proposed in this pull request:

Add a new prop to MultistepDialog which allows the positioning of the navigation element to the top or right.

Reviewers should focus on:

This is all done through style changes by applying a top level class for each position. All the overrides are in .#{$ns}-multistep-dialog-panels so it's all in one place, but alternative is to add position overrides to each existing class name where they appear.

Screenshot

Position.TOP

Position.RIGHT

blueprint-bot · 2021-10-19T20:21:20Z

Oops remove old props

Previews: documentation | landing | table | modern colors demo

mud · 2021-10-19T20:34:22Z

Hey @adidahiya I merged develop and clean up this PR in case you reconsider the position feature. Feel free to close if you don't think it's worthwhile to support -- I can create a custom component in-app instead.

aycai · 2021-10-19T22:34:46Z

Chatted with the original designer for some of her thoughts and relaying here along with mine:

Overall adding positioning seems reasonable. I'd say having the steps on the right side should be used with caution (for information hierarchy concerns) but some edge cases (e.g. RTL languages) might reasonably use this and at the end of the day it's up to each consumer.
I'd want to keep the header present with Position.TOP and discourage removing it to keep the structure of the dialog intact, as well as to keep the "close" button present.
Position.TOP makes this more apparent, but the differing background colors feel wonky when positioned next to both the header (with the white background) and the body (with the light grey background). This might be a separate change to consider outside the scope of positioning, but flagging here.

mud · 2021-10-19T23:12:56Z

Thanks @aycai

To address your second point, we fixed issues some issues with omitting title in #4969. So there is a way to close the dialog without the title.

As for discouraging Position.RIGHT, I just added that option because it was easy and reasonable. Position.BOTTOM didn't make sense since it would conflict with the footer. I'm happy to work on RTL support, especially since we'd need to flip the footer navigation (which currently is not possible.)

What I would like to get from the Blueprint team is 1) what can we do in this PR for you to accept the Position option and 2) any additional FLUPs to work on (ie. RTL support) which I'm happy to implement.

aycai · 2021-10-20T00:21:37Z

Gotcha, didn't realize the hidden header is already existing behavior. Moving the close button makes sense in a regular dialog, though the hierarchy gets a little wonky if the steps are to the left and outside the footer but the button to close the entire dialog is in that footer.

As for this change no other flags on the design side!

adidahiya · 2021-10-20T13:26:34Z

@aycai the title was removable before, but there were some styling issues which likely discouraged that kind of usage of the component. We only fixed those styling issues yesterday in #4969, thereby adding "full" support for omitting a title from MultistepDialog. At this point, we still have a chance to go back and change that (we can make title required and always render the header); that change has not been released yet in @blueprintjs/core. However I feel like it's probably ok despite the small information hierarchy concern you mentioned. Lmk if you feel strongly against it, otherwise I'll go ahead and review this PR soon...

aycai · 2021-10-20T23:35:31Z

I think we can keep our path forward here as is; the information hierarchy point is something I'm more concerned about enforcing separately.

…g-position

blueprint-bot · 2022-04-05T17:56:46Z

Merge remote-tracking branch 'origin/develop' into to/multistep-dialog-position

Previews: documentation | landing | table | demo

adidahiya · 2022-04-05T17:58:29Z

packages/core/src/common/classes.ts

+export const MULTISTEP_DIALOG_POSITION_TOP = `${MULTISTEP_DIALOG}-position-top`;
+export const MULTISTEP_DIALOG_POSITION_RIGHT = `${MULTISTEP_DIALOG}-position-right`;


Suggested change

export const MULTISTEP_DIALOG_POSITION_TOP = `${MULTISTEP_DIALOG}-position-top`;

export const MULTISTEP_DIALOG_POSITION_RIGHT = `${MULTISTEP_DIALOG}-position-right`;

export const MULTISTEP_DIALOG_NAV_TOP = `${MULTISTEP_DIALOG}-nav-top`;

export const MULTISTEP_DIALOG_NAV_RIGHT = `${MULTISTEP_DIALOG}-nav-right`;

adidahiya · 2022-04-05T17:58:45Z

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

@@ -23,6 +23,79 @@ $step-radius: $pt-border-radius * 2 !default;
      border-top-right-radius: $dialog-border-radius;
    }
  }
+
+  .#{$ns}-multistep-dialog-position-top & {


Suggested change

.#{$ns}-multistep-dialog-position-top & {

.#{$ns}-multistep-dialog-nav-top & {

adidahiya · 2022-04-05T17:58:57Z

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

+    }
+  }
+
+  .#{$ns}-multistep-dialog-position-right & {


Suggested change

.#{$ns}-multistep-dialog-position-right & {

.#{$ns}-multistep-dialog-nav-right & {

adidahiya · 2022-04-05T17:59:41Z

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

 import { DISPLAYNAME_PREFIX } from "../../common/props";
 import { Button, ButtonProps } from "../button/buttons";
 import { Dialog, DialogProps } from "./dialog";
 import { DialogStep, DialogStepId, DialogStepProps, DialogStepButtonProps } from "./dialogStep";

 type DialogStepElement = React.ReactElement<DialogStepProps & { children: React.ReactNode }>;

+export type MultistepDialogNavigationPosition = typeof Position.TOP | typeof Position.LEFT | typeof Position.RIGHT;


Suggested change

export type MultistepDialogNavigationPosition = typeof Position.TOP | typeof Position.LEFT | typeof Position.RIGHT;

export type MultistepDialogNavPosition = typeof Position.TOP | typeof Position.LEFT | typeof Position.RIGHT;

adidahiya · 2022-04-05T17:59:59Z

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

+     *
+     * @default Position.LEFT
+     */
+    navigationPosition?: MultistepDialogNavigationPosition;


Suggested change

navigationPosition?: MultistepDialogNavigationPosition;

navPosition?: MultistepDialogNavPosition;

adidahiya · 2022-04-05T18:00:13Z

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

@@ -45,6 +47,13 @@ export interface IMultistepDialogProps extends DialogProps {
     */
    finalButtonProps?: Partial<ButtonProps>;

+    /**
+     * Position of the step navigation.


Suggested change

* Position of the step navigation.

* Position of the step navigation within the dialog.

blueprint-bot · 2022-04-05T18:20:50Z

minor renames for brevity

Previews: documentation | landing | table | demo

mud added 8 commits October 15, 2021 12:42

Add has title option to example

0f027f3

Ensure rounded corners for no title

5cf1c34

Add close button to the footer

b4bdc6d

Expose footer close button option in example

fb896da

Add position option

064c07b

Expose position in example

b95f99d

Don't render border for first element

4926f51

Don't use bp-ns

83fdc8f

mud requested a review from adidahiya October 18, 2021 16:49

Base automatically changed from to/multistep-dialog-fixes to develop October 19, 2021 17:54

mud added 2 commits October 19, 2021 13:13

Merge branch 'develop' into to/multistep-dialog-position

33cc608

Oops remove old props

2571290

adidahiya added this to the core v4.1 milestone Mar 16, 2022

adidahiya self-assigned this Mar 16, 2022

adidahiya removed this from the core v4.1 milestone Apr 5, 2022

Merge remote-tracking branch 'origin/develop' into to/multistep-dialo…

7f48872

…g-position

minor renames for brevity

6afbd7f

adidahiya added this to the core v4.1 milestone Apr 5, 2022

adidahiya approved these changes Apr 5, 2022

View reviewed changes

adidahiya changed the title ~~Add positioning of navigation element for MultistepDialog~~ [core] feat(MultistepDialog): navigationPosition prop Apr 5, 2022

adidahiya merged commit d01d91c into develop Apr 5, 2022

adidahiya deleted the to/multistep-dialog-position branch April 5, 2022 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] feat(MultistepDialog): navigationPosition prop #4970

[core] feat(MultistepDialog): navigationPosition prop #4970

mud commented Oct 15, 2021 •

edited

Loading

blueprint-bot commented Oct 19, 2021

mud commented Oct 19, 2021

aycai commented Oct 19, 2021

mud commented Oct 19, 2021

aycai commented Oct 20, 2021

adidahiya commented Oct 20, 2021

aycai commented Oct 20, 2021

blueprint-bot commented Apr 5, 2022

adidahiya Apr 5, 2022

adidahiya Apr 5, 2022

adidahiya Apr 5, 2022

adidahiya Apr 5, 2022

adidahiya Apr 5, 2022

adidahiya Apr 5, 2022

blueprint-bot commented Apr 5, 2022

		export const MULTISTEP_DIALOG_POSITION_TOP = `${MULTISTEP_DIALOG}-position-top`;
		export const MULTISTEP_DIALOG_POSITION_RIGHT = `${MULTISTEP_DIALOG}-position-right`;

	.#{$ns}-multistep-dialog-position-top & {
	.#{$ns}-multistep-dialog-nav-top & {

	.#{$ns}-multistep-dialog-position-right & {
	.#{$ns}-multistep-dialog-nav-right & {

	export type MultistepDialogNavigationPosition = typeof Position.TOP \| typeof Position.LEFT \| typeof Position.RIGHT;
	export type MultistepDialogNavPosition = typeof Position.TOP \| typeof Position.LEFT \| typeof Position.RIGHT;

	navigationPosition?: MultistepDialogNavigationPosition;
	navPosition?: MultistepDialogNavPosition;

	* Position of the step navigation.
	* Position of the step navigation within the dialog.

[core] feat(MultistepDialog): navigationPosition prop #4970

[core] feat(MultistepDialog): navigationPosition prop #4970

Conversation

mud commented Oct 15, 2021 • edited Loading

Follow up to #4969

Checklist

Changes proposed in this pull request:

Reviewers should focus on:

Screenshot

Position.TOP

Position.RIGHT

blueprint-bot commented Oct 19, 2021

Oops remove old props

mud commented Oct 19, 2021

aycai commented Oct 19, 2021

mud commented Oct 19, 2021

aycai commented Oct 20, 2021

adidahiya commented Oct 20, 2021

aycai commented Oct 20, 2021

blueprint-bot commented Apr 5, 2022

Merge remote-tracking branch 'origin/develop' into to/multistep-dialog-position

adidahiya Apr 5, 2022

Choose a reason for hiding this comment

adidahiya Apr 5, 2022

Choose a reason for hiding this comment

adidahiya Apr 5, 2022

Choose a reason for hiding this comment

adidahiya Apr 5, 2022

Choose a reason for hiding this comment

adidahiya Apr 5, 2022

Choose a reason for hiding this comment

adidahiya Apr 5, 2022

Choose a reason for hiding this comment

blueprint-bot commented Apr 5, 2022

minor renames for brevity

mud commented Oct 15, 2021 •

edited

Loading