chore(Wizard): make wizard inPage #4140

nicolethoen · 2020-04-28T14:08:31Z

Summary of changes:

To Modal component

Uses pf-c-modal-box__title in the modal header if title is provided.
Make title prop optional.
Add aria-label prop for use if title is not provided.
Add noPadding prop to place Modal children in the ModalContent rather than a ModalBody
Add 'Modal with wizard' and 'No title' example to docs

To Wizard component

Only display wizard header if a title is provided.
Added hideClose prop to allow user to hide the close button on wizard header.
remove wizard's inPage prop.
Update wizard examples. Add in page example showing wizard header with no close button. Add - Wizard in modal example.
implemented Wizard's height prop.

Above changes address Core updates:
patternfly/patternfly#2969
patternfly/patternfly#2964

What: Closes #3668

Breaking changes

Modal: Remove hideTitle prop. To hide the Modal header, do not pass a title prop, a description prop, or a header prop. If there is no title or header passed, please provide an aria-label prop to the Modal component to make it accessible.
Wizard: Remove inPage prop. By default the Wizard will be displayed in page, filling its parent container. If the consumer passes a managed isOpen flag, then the Wizard will be displayed in a modal.

patternfly-build · 2020-04-28T14:19:31Z

PF4 preview: https://patternfly-react-pr-4140.surge.sh
A11y report: https://patternfly-react-pr-4140-coverage.surge.sh

dlabrecq

You can see the adverse effect of the wizard changes in our examples

dlabrecq · 2020-04-28T14:20:53Z

packages/react-core/src/components/Wizard/Wizard.tsx

-  isOpen?: boolean;
-  /** True to show the wizard without the modal */
-  isInPage?: boolean;


I don't understand why isOpen was removed? Are all wizards now being placed within a modal?

nicolethoen · 2020-04-28T14:36:17Z

@dlabrecq
Yeah, this is a breaking change PR for sure.
The wizard is just a regular component now, filling its parent element. So the steps scroll in the examples for Wizard.

To see the Wizard component in a Modal, there is now an example in the Modal component examples. I could conceivably add it to the wizard examples as well.

The issue I addressed wanted to decouple the Wizard with the Modal component so that it wasn't a second modal implementation.

nicolethoen · 2020-04-28T14:43:02Z

@tlabaj
I added a variant to the Modal so that it can now house a Wizard - you can see it in the examples.
Do we want a variant to the Wizard that puts it in a modal? Do we want variants going both directions?
Easy enough to add. If I add it, do we want it to be in a modal by default? If it is by default - are these changes necessary?

redallen

I'm liking where this is going~! You can create a wizard.css file with a rule like

.ws-react-c-wizard {
  min-height: 400px;
}

and just import it in Wizard.md to fix the height of the examples in the preview :)

redallen · 2020-04-28T14:49:00Z

packages/react-core/src/components/Wizard/Wizard.tsx

-      this.descriptionId = `pf-wizard-description-${newId}`;
-    }

    this.state = {


If all you're doing is setting state, you don't need a constructor and can just set the class member state.

redallen · 2020-04-28T14:59:11Z

As for the API, I'm fine with whatever we choose since there aren't that many products using a Wizard:

My 2 cents are that we should create a wrapper WizardModal component and export that. I could more easily write codemods for that.

mcoker · 2020-04-28T15:51:39Z

This looks great @nicolethoen! A couple of things. In the example where we show the wizard in a modal

the large modal size variation is preferred for a wizard in a modal. Can we update the example to use size={ModalVariant.large}?
In a modal, the wizard header should be present. That will also introduce a close button.
Is disableFocusTrap={true} required with isBasic if there isn't a close button? If I don't add that to the examples, I get an error "You can't have a focus-trap without at least one focusable element". I wonder if we need to call any of that out in the docs?

You can see examples of the size variation and wizard header in the core docs under demos -> wizard - http://patternfly-v4.surge.sh/documentation/core/demos/wizard

nicolethoen · 2020-04-28T16:38:37Z

In a modal, the wizard header should be present. That will also introduce a close button.

Is disableFocusTrap={true} required with isBasic if there isn't a close button? If I don't add that to the examples, I get an error "You can't have a focus-trap without at least one focusable element". I wonder if we need to call any of that out in the docs?

These are the sorts of things that would actually be solved much easier with a WizardModal wrapper. Because the Wizard needs to know it's in a modal to have these features work correctly. It cannot remain parent agnostic. I will begin implementing a WizardModal component as Zac recommended.

nicolethoen · 2020-04-29T12:51:39Z

In a modal, the wizard header should be present. That will also introduce a close button.

Is disableFocusTrap={true} required with isBasic if there isn't a close button? If I don't add that to the examples, I get an error "You can't have a focus-trap without at least one focusable element". I wonder if we need to call any of that out in the docs?

These are the sorts of things that would actually be solved much easier with a WizardModal wrapper. Because the Wizard needs to know it's in a modal to have these features work correctly. It cannot remain parent agnostic. I will begin implementing a WizardModal component as Zac recommended.

So, I think I was able to get it to work. If the consumer provides a Title, the WizardHeader will appear. So a Wizard with a Title placed in a Modal with the isBasic flag works like an old Wizard :)

karelhala · 2020-04-30T07:28:46Z

As an app developer, I wouldn't think to look at the modal component for wizard examples. The previous wizard examples had shown how to launch the wizard.

The wizard component was from the start fundamentally wrong, it's not specific usecase only to modals. It should be a component that can be placed wherever you want and be free with it. Why not placing the wizard component in dropdown? It might sound strange, but the useage of it can be actually pretty nice. As for the demo, the wizard page demo could have an example in modal since that's one usecase, but I wouldn't limit the wizard component to modals only.

I think this is great direction with the wizard compoent, thank you @nicolethoen for taking care of it!

karelhala · 2020-04-30T07:35:33Z

packages/react-core/src/components/Modal/ModalContent.tsx

-        {children}
-      </ModalBoxBody>
-      {modalBoxFooter}
+      {!isBasic && showClose && <ModalBoxCloseButton onClose={onClose} />}


Can we remove this isBasic prop and unify all these elements into one component?

const ModalBody = () => { onst modalBoxHeader = header ? ( <div className={css(titleStyles.title)}>{header}</div> ) : ( <ModalBoxHeader hideTitle={hideTitle}> {title} </ModalBoxHeader> ); const modalBoxFooter = footer ? ( <ModalBoxFooter>{footer}</ModalBoxFooter> ) : ( actions.length > 0 && <ModalBoxFooter>{actions}</ModalBoxFooter> ); return <Fragment> {showClose && <ModalBoxCloseButton onClose={onClose} />} {modalBoxHeader} {description && <ModalBoxDescription id={id}>{description}</ModalBoxDescription>} <ModalBoxBody {...props} {...(!description && { id })}> {children} </ModalBoxBody> {modalBoxFooter} </Fragment> }

This way we won't have a ton of ifs in this component and it can just have children as with any other component.

If we don't want to break backwards compatibility that much we could add prop called bodyWrapper to Modal where you'd by default use ModalBody component, if you'd don't want to use you could pass in null or w/e. The bodyWrapper prop could be either null, string, number, function or node.

karelhala · 2020-04-30T15:10:07Z

I've had discussion with a few folks about this PR, I really want it to be part of PF, because it can be great example of how to write and export componets as building blocks. However this can eventually be hard to migrate to as we usually have a lot of other things to do instead just fixing breaking changes from component libray.

This PR fixes the linked issue in elegant way, but it will introduce a breaking change and would require from app developers to fix this. There can be another approach to this, it will bloat the PR, but I think it'll help us in the long run:

Add new component exported as Wizard that will be just a wrapper that passes down all props except isModal. Based on this prop it'll use Wizard in modal (current way) or modal in page without modal at all (new and proposed way)
It would be also beneficial to export a few new components as proposed in the original issue. App devs can then combine these components toger as they choose and as they want.
- WizardHeader
- WizardStepper
- WizardBody
- WizardInPage - combination of WizardHeader, WizardStepper and WizardBody. This component can by default have these 3 components used trough render funcion so consumers can override these part with their own. Meaning if I don't want to use WizardStepper and I want to implement my own I can do that easilly by adding wizardStepper={(...props) => <div>My awesome stepper</div>}.

@nicolethoen the work you've done here is awesome, it actually led me to these ideas and proposals, without this change we'd probably won't know what would be the ideal outcome of original issue. But I feel like this is the right path. I'd be more than happy to implement it as well, because this might be interesting addition. That being said if you'd need any more information or didn't understood anything, don't hesitate to ask. I'll monitor this PR and try help you as much as I could so we can have this change un in the wilderness soon.

jschuler · 2020-05-04T14:08:15Z

packages/react-core/src/components/Modal/Modal.tsx

  /** Simple text content of the Modal Header, also used for aria-label on the body */
-  title: string;
-  /** Flag to hide the title */
-  hideTitle?: boolean;


This looks great! Can you please add all the breaking changes to the bottom of your PR? in this form:

## Breaking changes - Your list of changes. - You can use MD

jschuler · 2020-05-04T14:09:43Z

packages/react-core/src/components/Modal/examples/Modal.md

 }
 ```
+
+```js title=No-title


the no header and no title examples seem identical?

Lol, yep. That's evident of me experiencing tunnel vision.

jschuler · 2020-05-04T14:11:49Z

packages/react-core/src/components/Modal/examples/Modal.md

+}
+```
+
+```js title=With-wizard


The Escape button does not seem to be working to close the wizard.

codecov-io · 2020-05-04T17:54:11Z

Codecov Report

Merging #4140 into v4 will decrease coverage by 0.79%.
The diff coverage is 73.68%.

@@            Coverage Diff             @@
##               v4    #4140      +/-   ##
==========================================
- Coverage   58.73%   57.94%   -0.80%     
==========================================
  Files         396      390       -6     
  Lines        6007     5963      -44     
  Branches     2350     2334      -16     
==========================================
- Hits         3528     3455      -73     
- Misses       2022     2060      +38     
+ Partials      457      448       -9

Flag	Coverage Δ
#patternfly4	`57.94% <73.68%> (-0.80%)`	⬇️

Impacted Files	Coverage Δ
...tension/src/components/CatalogTile/CatalogTile.tsx	`79.16% <ø> (ø)`
packages/react-core/src/helpers/util.ts	`46.66% <ø> (ø)`
...es/react-table/src/components/Table/RowWrapper.tsx	`67.85% <ø> (-1.11%)`	⬇️
...src/components/Table/utils/decorators/cellWidth.ts	`100.00% <ø> (ø)`
...-table/src/components/Table/utils/transformers.tsx	`100.00% <ø> (ø)`
...kages/react-core/src/components/Modal/ModalBox.tsx	`55.55% <33.33%> (-11.12%)`	⬇️
...eact-core/src/components/Toolbar/ToolbarFilter.tsx	`8.00% <50.00%> (ø)`
.../react-core/src/components/Toolbar/ToolbarItem.tsx	`60.00% <50.00%> (ø)`
...core/src/components/Toolbar/ToolbarToggleGroup.tsx	`23.52% <50.00%> (ø)`
...ckages/react-core/src/components/Wizard/Wizard.tsx	`44.52% <53.84%> (-1.37%)`	⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ed8b95...44dbf15. Read the comment docs.

jschuler · 2020-05-04T18:10:45Z

packages/react-core/src/components/Modal/ModalContent.tsx

    <div className={css(titleStyles.title)}>{header}</div>
  ) : (
-    <ModalBoxHeader hideTitle={hideTitle}> {title} </ModalBoxHeader>
+    title && <div className={css(styles.modalBoxTitle)}>{title}</div>


I think it might be good to keep the ModalBoxHeader and just update it. This way, existing consumers won't be too broken

…izard

…n is defined

jschuler

LGTM!

redallen

This API is a crowd pleaser!

Examples had min-height added.

redallen · 2020-05-04T19:59:41Z

packages/react-core/src/components/Wizard/Wizard.tsx

-        target.appendChild(this.container);
-      }
-      this.toggleSiblingsFromScreenReaders(true);
+    const target = (typeof document !== 'undefined' && document.body) || null;


Creative way to write a ternary.

lol I guess I too quickly borrowed the logic from a different part of the code :P

mcoker · 2020-05-04T20:52:01Z

packages/react-core/src/components/Modal/Modal.tsx

  /** Variant of the modal */
  variant?: 'small' | 'large' | 'default';
+  /** Flag indicating if modal content should have no padding*/
+  noPadding?: boolean;


I wonder if this prop name is too generic and might overlap or be confusing with other props we could introduce into the modal, like one that removes all padding from the modal, or just the padding from the <ModalBoxBody> element. I have seen potential use cases for the former.

Technically this prop removes the <ModalBoxBody> element all together.

Here is what it looks like now with regular modal content and noPadding

hm... would it makes sense to have it be called noModalBoxBody? Would that make sense to a consumer? Could something like removeBody make more sense?

Yeah, noModalBoxBody definitely isn't as easy of a name, though it's a more accurate description of what it does. Or maybe noModalBoxBodyWrapper since you can still pass content and it puts it where the body would go. noModalBoxBody and removeBody could be confused with the modal having no body content. At any rate, I think as long as we document it, any of those names seem OK to me. I'm interested in others' thoughts.

I could go with noModalBoxBodyWrapper... it's a mouthful.
@tlabaj @jschuler @dlabrecq any thoughts?

mcoker · 2020-05-04T20:52:45Z

Should we remove the description prop from wizard examples that don't also have a title, since description will only show up if you have a title?

mcoker

Looks good to me! Thanks for all of your hard work on this @nicolethoen!

karelhala

Looking good! I am wondering if we could also somehow export helper functions this.goToStepById, this.goToStepByName, this.onNext and this.onBack. No need to be part of this PR at all! But it would make the further usage of smaller building blocks much easier.

redallen

I love seeing more lines removed than added :)

Thanks for the refactor @nicolethoen !

patternfly-build · 2020-05-05T19:39:28Z

Your changes have been released in:

@patternfly/react-catalog-view-extension@4.2.18
@patternfly/react-core@4.9.12
@patternfly/react-docs@5.2.22
@patternfly/react-inline-edit-extension@4.3.18
demo-app-ts@4.6.15
@patternfly/react-integration@4.6.5
@patternfly/react-table@4.3.18
@patternfly/react-topology@4.2.18
@patternfly/react-virtualized-extension@4.2.18

Thanks for your contribution! 🎉

dlabrecq previously requested changes Apr 28, 2020

View reviewed changes

nicolethoen requested review from mcarrano, mcoker and tlabaj April 28, 2020 14:36

redallen suggested changes Apr 28, 2020

View reviewed changes

tlabaj added the Breaking change 💥 this change requires a major release and has API changes. label Apr 28, 2020

nicolethoen force-pushed the wizard_modal_changes branch from 92ade72 to 72d3427 Compare April 29, 2020 12:46

karelhala reviewed Apr 30, 2020

View reviewed changes

nicolethoen force-pushed the wizard_modal_changes branch from 72d3427 to 2bd7bfe Compare May 1, 2020 16:19

jschuler reviewed May 4, 2020

View reviewed changes

jschuler suggested changes May 4, 2020

View reviewed changes

nicolethoen force-pushed the wizard_modal_changes branch from 7dae4e1 to 3df306c Compare May 4, 2020 16:11

jschuler reviewed May 4, 2020

View reviewed changes

nicolethoen added 6 commits May 4, 2020 14:59

chore(Wizard): make wizard inPage, and modify modal so it can house w…

76d0f0f

…izard

fix lint issues

fb0200b

add WizardHeader to wizard if title is provided

1d3c734

update modal to use modal-box__title and put wizard in modal if isOpe…

7a0cf44

…n is defined

update comments

be25538

rebase and fix esc behavior

61e9b3f

nicolethoen added 2 commits May 4, 2020 14:59

fix lint issues

5fb68de

add ModalBoxHeader back

4fa074a

nicolethoen force-pushed the wizard_modal_changes branch from 44dbf15 to 4fa074a Compare May 4, 2020 19:01

jschuler approved these changes May 4, 2020

View reviewed changes

redallen approved these changes May 4, 2020

View reviewed changes

redallen reviewed May 4, 2020

View reviewed changes

mcoker reviewed May 4, 2020

View reviewed changes

remove unnecessary descriptions from examples

21d2de5

mcoker approved these changes May 4, 2020

View reviewed changes

clean up logic

8d7904f

karelhala approved these changes May 5, 2020

View reviewed changes

redallen approved these changes May 5, 2020

View reviewed changes

redallen merged commit a6f4431 into patternfly:v4 May 5, 2020

jessiehuff mentioned this pull request May 8, 2020

Modal title and label missing warning #4218

Closed

jschuler mentioned this pull request Jun 3, 2020

Wizard: Remove inPage prop and Modal hideTitle prop patternfly/pf-codemods#79

Closed

nicolethoen deleted the wizard_modal_changes branch February 8, 2023 13:49

chore(Wizard): make wizard inPage #4140

chore(Wizard): make wizard inPage #4140

Uh oh!

Conversation

nicolethoen commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes:

To Modal component

To Wizard component

Breaking changes

Uh oh!

patternfly-build commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

dlabrecq Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolethoen commented Apr 28, 2020

Uh oh!

nicolethoen commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

redallen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redallen commented Apr 28, 2020

Uh oh!

mcoker commented Apr 28, 2020

Uh oh!

nicolethoen commented Apr 28, 2020

Uh oh!

nicolethoen commented Apr 29, 2020

Uh oh!

karelhala commented Apr 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karelhala commented Apr 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 4, 2020

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jschuler left a comment

Choose a reason for hiding this comment

Uh oh!

redallen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker commented May 4, 2020

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

nicolethoen commented Apr 28, 2020 •

edited

Loading

patternfly-build commented Apr 28, 2020 •

edited

Loading

dlabrecq Apr 28, 2020 •

edited

Loading

nicolethoen commented Apr 28, 2020 •

edited

Loading