Patternfly-react: PF4 Wizard issues

[rvsia]

Hello,

thank you for exporting all parts of PF4 Wizard.

However, there are several issues with implementation of this component:

1) The Wizard is another implementation of Modal.

PatternFly should reuse its own components and it should use the Modal component.

The wizard's render function starts with

isOpen && ReactDOM.createPortal(
        <FocusTrap focusTrapOptions={{ clickOutsideDeactivates: true }}>
          <Backdrop>
            <Bullseye>

Instead of that it should be wrapped only the Modal component. (_And Modal component needs to be updated to be able to use here._) Or a consumer should be able to put modal there in his own implementation, if he wants to do. There is no reason to control this in the Wizard component, when you have another component, which does it. ( => Less code to test => less code to maintain => less code to break => more satisfied consumers and happy faces)

<Modal isOpen>
  ...
</Modal>

or in user's implementation

<Modal isOpen>
   <Wizzard {...props} />
</Modal>

2) Structure

The structure of wizard is confusing:

<div>
  <WizardHeader />
  <WizardToggle >
     <WizardFooter />
  </WizardToggle>
</div>

_Where is the body? Where is the nav? Why is the footer in the toggle?_

The structure should be more clear and readable, something like this:

<WizzardWrapper>
  <WizardHeader />
  <WizardNav>
    <WizardNavItem />
     ...
  </WizardNav>
  <WizardBody>
     ...
  </WizardBody>
  <WizardFooter>
    ...
  </WizardFooter>
</WizzardWrapper>

A structure like this would allow all consumers to build their own implementations because...

3) Wizard is too controlled

In our project, we have a specific approach how to build and render forms (and wizard forms). However, our approach can't be adjusted to use how this component is designed. So, I had to implement my own Wizard PF4: https://github.com/data-driven-forms/react-forms/blob/b9bef870c4755a4436290df359a6a965f4c6236a/packages/pf4-component-mapper/src/form-fields/wizard/wizard.js

Individual parts of wizard should be self-contained. There could be a second Wizard component, which would be controlled by PatternFly, but there should be a set of basic components which provides styles and very basic render logic.

I would like to get WizardWrapper, WizardHeader, WizardNav, WizardNavItem, WizardBody, WizardFooter and build my own logic around it. (Because we are using Wizards in a specific way and I believe we are not the only ones.)

4) There are string props

Title, descriptions, button labels, all these props should be React nodes to allow put there anything (ex.: localized text with using of react-intl, or titles with a tooltip). Only string props should be accessibility props (aria-label, etc.) where any html elements cannot be rendered .

cc @Hyperkid123 @karelhala @martinpovolny

[martinpovolny]

title, descriptions, button labels, all these props should be React nodes to allow put there anything

+1 on that. That is a very real issue. We are trying to have new code fully i18n-ed. This makes it more complicated.

We have problems using the new PF wizard in Data-driven-forms which is a pity.

Also looking forward at some designs for the following versions of ManageIQ we have for example a wizard-style interaction that is whole screen, because it is very complex. (A design from @terezanovotna .) I would prefer such interaction NOT to be inside a Modal, because why when it is full-screen. So this again limits the possibility of using the original PF4 component there.

@rvsia : could you, please prepare a CodeSandbox with your improved Wizard component so that people (not only) from Patternfly can take a look and play with it easily?

Could we replace the Patternfly wizard with Richard's implementation? I mean do it before there's too much code already written using the problematic component?

[rvsia]

@martinpovolny After the PR is merged, anyone can play with it here

I am not sure if DDF Wizard should replace anything here, it's designed for using with our forms, which is only a alternative way how to build/render them, maybe other people want to use totally different approach (formik, form-hooks, etc.).. and that is OK and that is exactly why PF should provide basic components => to allow anyone to do anything. My rewritten wizards parts are basically <div>s with some classes on them. However, because of accessibility and number of props it's not suitable for long-term maintenance and it would be best to have them in this package, where PF team could take care of them and do it correctly.

[Hyperkid123]

could you, please prepare a CodeSandbox with your improved Wizard component so that people (not only) from Patternfly can take a look and play with it easily?

@martinpovolny that is not the bes way how to show of proposed changes because there is some logic baked in.

What we are after are dumb styled components from which we can create wizard. Same as if you would build it using just html + css. That is basically how its recreated in data driven forms.

[martinpovolny]

I am not sure if DDF Wizard should replace anything here

I see now that you did not implement the proposed structure, but instead did what was necessary to use the PF4 wizard styling inside the DDF. So, right the suggestion does not make sense.

[tlabaj]

Issue #2046 has been opened in core to address issue number 1 in the list.

[rachael-phillips]

I broke out 2 and 3 here: https://github.com/patternfly/patternfly-react/issues/2592
and 4 here: https://github.com/patternfly/patternfly-react/issues/2593

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[rvsia]

@rachael-phillips @tlabaj I see that all issues are marked stale and wontfix now. So, what's the status? Should I expect any or no changes? Thanks.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[karelhala]

@rachael-phillips are there any blockers to make the wizard not be it's own implementation of dialog? From the code this line Wizard.tsx#L520 can be very much replaced by using Modal.tsx. When this component will be used tons of custom logic for dialog behavior will be removed as well. And on top of that once we will allow closing dialogs when clicking outside container this will part of wizard as wel.

[Hyperkid123]

@karelhala i would add one more question. Does the wizard has to be always a modal? Would not it be better if developers could compose it from existing components?

[martinpovolny]

Does the wizard has to be always a modal?

According to design that I have got: I need a wizard w/o a modal.

[rvsia]

@Hyperkid123 https://www.patternfly.org/v4/design-guidelines/usage-and-behavior/wizard#in-page-wizards

[rachael-phillips]

@tlabaj what do you think?

[jschuler]

@martinpovolny The Wizard has a prop isInPage that you can set to true

[karelhala]

@jschuler looping back to my original question - are there any blockers to make the wizard not be it's own implementation of dialog.

It feels really dull to have same implementation full of ifs just to render component outside and inside dialog. Wouldn't it be easier to write the wizard on it's own as always in page and let consumers add it to dialog. We can help them by doing this for themselves, but with importing dialog and placing wizard in it, not by reimplementing dialog again just for wizard. I know that the classnames might be a bit off, if so we should loop back to style core repo and tell them to implement only in-page wizard.
There is unnecessary logic just to distinguish between in page and in dialog wizard, plus on top of this another logic to operate dialog. This leads to repeating bugs, there might be in future time when we decide that dialogs can be closed when user clicks outside of it. We'd have to write this logic twice - once for dialog; another time for wizard. That is not good, repeating code is not great and PF should aim to remove as much duplicity as possible.

[jschuler]

@karelhala it's a good suggestion, not sure if the Modal component is generic enough yet, might need to rewrite it a bit to allow Wizard to be placed inside, if not a more generic Dialog component that could be used by Modal as well could be created. Not sure if we can ask designers to make the Wizard look the same in-page and in dialog, but if the Dialog was to provide a context variable then Wizard could look for it and know if it's inside a dialog and append classes as needed. So it sounds feasible, if you'd like to open an issue for this we can track this work and implement it with the next breaking change release (around end of April/beginning of May I think)

[jschuler]

@rvsia To change the structure would require changes to the HTML/CSS in core as well. Would you be okay if you could customize and pass WizardHeader, WizardFooter, WizardNav etc to a container (Wizard or a new WizardContainer) as props instead of children instead?
e.g.

<WizardContainer
  header={
    <WizardHeader
      title={"WizardHeader title"}
      description={"WizardHeader description"}
    />
  }
  nav={
    <WizardNav>
      <WizardNavItem
        text={"Item 1"}
        onNavItemClick={() => updateBodyContent(0)}
      />
      <WizardNavItem
        text={"Item 2"}
        onNavItemClick={() => updateBodyContent(1)}
      >
        <WizardNav>
          <WizardNavItem
            text={"Item 2 - A"}
            onNavItemClick={() => updateBodyContent(2)}
          />
          <WizardNavItem
            text={"Item 2 - B"}
            onNavItemClick={() => updateBodyContent(2)}
          />
        </WizardNav>
      </WizardNavItem>
    </WizardNav>
  }
  body={
    <WizardBody>{currentStep}</WizardBody>
  }
  footer={
    <WizardFooter>
      <Button>Next</Button>
      <Button>Previous</Button>
    </WizardFooter>
  }
/>

I don't see an advantage for inserting them as children and this way we could keep the current HTML/CSS structure. What do you think?

[rvsia]

WizardContainer is just a styling component with no functionality? If so, then I agree with this. However, for consistency reasons, wouldn't it be better to pass the body as a child, because then it would be the same interface as for the modal component?

[jschuler]

Yes with no functionality and ideally no state management, just styling and layout out the DOM structure while giving consumer all the power. Body could definitely be a child that makes sense

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.