docs(Wizard/next): Recreate all legacy wizard examples

[jpuzz0]

What: Closes #8398
Additional issues: #8116

Had to adjust some things in order to get all examples to properly work. Converted all to functional components and to use typescript. Also, tried to improve examples where it made sense to, as a few were misleading.

Some other minor changes include:

• WizardControlStep renamed to WizardStepType (as the meaning of "control" is probably not obvious and requires an explanation).
• Renamed functions returned from context to more accurately represent what will happen when called (e.g. onNext -> goToNextStep, onClose -> close).
• Renamed isStepVisitRequired to isVisitRequired (Inclusion of "Step" seems implied).
• Introduced new prop called isProgressive to incorporate built-in ability described in our design guidelines here (Added unit test for this as well).
• Combined onNavByIndex, onNext, & onBack into a single onStepChange prop since their interfaces were all the same and to simplify the Wizard's API. Introduced WizardStepChangeScope so users can still perform operations based on whether they click Next, Back or a nav item.

As a part of this change, I've also included a type change for MenuGroup, since the demo-app build was failing. The type was updated for label to be just ReactNode and not ReactNode | FC, where the issue was that the only example using a FC for the label prop had properties of its own, and unless specified in the type (e.g. ReactNode | FC<{ someFcProp: string }>) I wouldn’t expect the MenuDemo (which uses MenuGroup) to compile; ab34e5a

[patternfly-build]

Preview: https://patternfly-react-pr-8422.surge.sh

A11y report: https://patternfly-react-pr-8422-a11y.surge.sh

[mcarrano]

Assuming that these changes apply to the React next implementation? I did give this a quick look and don't see any issues, however I'd like @mmenestr to also take a look.

With the number of different examples here, I think this calls for further documentation of what each example is intended to show. It's hard for me to know from just the example titles. @jeffpuzzo is that something you think you could work with @edonehoo to add? We can discuss. Or perhaps that can be a follow up PR.

[nicolethoen]

I created this follow up issue to track the addition of documentation to the examples :#8499

[nicolethoen]

surprised I didn't notice this before, but is there another way we can accomplish something similar without needing to rely on lodash? that's a third party dependency we are trying to divorce ourselves from.

[jpuzz0]

Yes, we can accomplish this another way. JS is supposed to natively support the function, but I don't think all browsers support findLast and findLastIndex quite yet, and we'd have to change our tsconfig to add the latest lib ES version, which we can do another time.

[jpuzz0]

This has been addressed using a combination of reverse & find, which leads to an additional iteration, but not one that should be very taxing performance-wise. Also, once findLastIndex is supported we can just switch to using that function and have 1 iteration again for goToPrevStep.

[tlabaj]

Good call out Nicole. Can we make the update to not use lodash please.

[jpuzz0]

@tlabaj This update was made yesterday. This change is outdated.

[nicolethoen]

@tlabaj can correct me if I'm off, but I think we are trying to standardize all callback prop types exposed to the consumers across the library so that they all pass the event back as the first parameter.

[jpuzz0]

That would certainly complicate things if that were true for this case. This function is a consolidation of 3 that were already defined as a type without an event handler bound as the first parameter, so I wonder if this could be handled separately as a part of the broader effort to make this change in PF?

[tlabaj]

Hi @jeffpuzzo we are trying to standardize your callback signatures so that an event is always passed and it is the first parameter. This was an ask from consumers. Some consumers have to wrap PF components in order to use 3rd party libs like Formik due to our inconsistent/non standard following callbacks.

I am ok with opening a separate issue to tackle that, but we would want to get that in for our major release.

[jpuzz0]

@tlabaj @nicolethoen I ended up just trying to add those event props as a part of this PR. Please have a look when you have a moment.

[mmenestr]

From a functional and visual standpoint this looks good to me!

[nicolethoen]

@jeffpuzzo
I just opened this PR which ensures that focus is put into the drawer when the drawer is expanded. The related examples in your PR likely need the same treatment.

[jpuzz0]

@jeffpuzzo I just opened this PR which ensures that focus is put into the drawer when the drawer is expanded. The related examples in your PR likely need the same treatment.

@nicolethoen Thanks for the heads up. I duplicated your logic inside of the example specifically for demonstrating drawers; examples/WizardWithDrawer.tsx

[mcoker]

LGTM! Just a couple of comments.

[mcoker]

FWIW the existing demo has helperText and this one doesn't - was that intentional?

patternfly-react/packages/react-core/src/components/Wizard/examples/WizardEnabledOnFormValidation.tsx

Lines 32 to 33 in 90e75e7

	helperText="Write your age in numbers."
	helperTextInvalid="Age has to be a number"

[jpuzz0]

Not intentional. I can add that.

[mcoker]

Placing an element here isn't supported by the component. It works, but I don't think we should recommend it. As far as putting custom stuff in the wizard elements, it should either go in the body or the footer, though you can't put it in the footer since it's only designed to hold actions. We could update it to support messaging like this (or to support whatever content you want in the footer) if we need to.

Also it seems like this error would go at the top per the alert design guidelines? There is an example of an inline alert at the bottom of the wizard content in the design guidelines, but it's for when the alert only pertains to a specific part of the content - I believe the suggestion is to put the alert inline with the content it pertains to. @mceledonia @mcarrano @mmenestr is that right?

I think the existing/non-next example is how I would expect it, but we should be using an inline error (like you've done it in this PR) instead of a toast warning alert. Here's the existing example:

[jpuzz0]

Admittedly, I did try to rework the existing example here to be a little more intuitive and useful. I felt as though the "Go to Beginning" button had no meaning for the example, so I moved the form input to step 1, and in doing so, felt like an Alert would be useful to show an error on the final step, where now "Going to Beginning" is necessary to correct the validation issue.

For now, I've moved the form input back to the final step, am showing the warning alert above the form on "Validate" click if the input is invalid, removed the status icon for step 1 and the error alert that used to show above the footer for the final step.

[mcoker]

I chatted with @mcarrano and confirmed it should be an inline error alert instead of a non-inline (toast) warning alert. The positioning is fine. Though it's not a huge deal, so no worries if we want to merge this as-is. But if it's an easy update, we could update that here and in the non-next example.

Also not a blocker but a nice to have, the actions could use updates - though they conflict with one another, depending on the wording we want to use:

• We shouldn't have 2 primary buttons in a row like that. "Go to beginning" could be a secondary button.
• "Go to Beginning" should be sentence case "Go to beginning".
• Unless there's a reason to take someone to the start of the form, "Go to beginning" could just be "Back" like the rest of the buttons.
• Do you know why these actions use "Forward" and "Backward" instead of "Next" and "Back" like the other examples?

[jpuzz0]

There doesn't seem to be any reason for the difference in naming for Next/Back or any reasoning for the existence of "Go to beginning", so I'm in favor of removing that button and changing to naming convention of Next/Back buttons.

[tlabaj]

WizardFooterWrapper should be added here.

[tlabaj]

Looking good Jeff. I do now wonder if some of the examples are now technically demos. (E.g. the example with drawer). It is showing how to add a drawer to a wizard step, but the drawer is not technically part of the component. Either way, it is not a blocker for me.

@mcarrano @margot what are your thoughts.

[tlabaj]

I am rethinking this prop. I am wondering if it should align with the old implementation and be called isExpandable. The way it is implemented changes the default behaviour of the original Wizard. The steps would be expandable by default here instead of having to opt into expandable?

@mcarrano @mmenestr any thought on this?

[mcarrano]

I don't feel strongly about what the default should be. My only concern is how this will effect existing consumers of the wizard if they transition to the Next component.

[tlabaj]

@mcarrano Switching to the Next component will require Consumers to reimplement their wizards. This prop is one of many API changes the consumer will need to make. So changing the name an default is not a big deal in the grand scheme of things.
I do think isExpandable is a more consistent name that we use for similar interactions in multiple components. So I would suggest that for name.
As for the default, my question for design is do we have s preference as to whether sub-steps should se expandable by default should that be opt in? (It was opt in for original wizard). If there is no preference, we can leave it as is in this PR.

[jpuzz0]

I went ahead and made the change to have it default to "false" for now. I don't have a preference and I can switch it back if we want.

[tlabaj]

👍 A great example of the new composable API benefits. The drawer no longer needs to be rendered by the component internally!

[mcoker]

Is there a reason this couldn't just be a "back" button? Maybe there is, but it feels like you're trapped on that step, even though you aren't and can still navigate using the nav.

[jpuzz0]

True. I can add a Back button, but they could just click the nav items at that point.

[mcoker]

Same here, could this just be a back button like the previous steps?

[mcarrano]

I do now wonder if some of the examples are now technically demos. (E.g. the example with drawer). It is showing how to add a drawer to a wizard step, but the drawer is not technically part of the component. Either way, it is not a blocker for me.

I don't feel strongly about this. I agree that this is a gray area and that yes, a wizard with a drawer does feel more like a demo since the drawer is separate from the wizard component.

[jpuzz0]

I do now wonder if some of the examples are now technically demos. (E.g. the example with drawer). It is showing how to add a drawer to a wizard step, but the drawer is not technically part of the component. Either way, it is not a blocker for me.

I don't feel strongly about this. I agree that this is a gray area and that yes, a wizard with a drawer does feel more like a demo since the drawer is separate from the wizard component.

@tlabaj @mcarrano My question for now is, should we just remove this example or comment it out for now? Or are we comfortable exposing this as an example with the possibility of it later being deemed a demo and we just move it later?

[mcoker]

🚀

[mcarrano]

My question for now is, should we just remove this example or comment it out for now? Or are we comfortable exposing this as an example with the possibility of it later being deemed a demo and we just move it later?

I am OK if this example stays. We can always rework this later for the v5 release. What do you think @tlabaj ?

Also, I thought you were updating this example @jeffpuzzo (https://patternfly-react-pr-8422.surge.sh/components/wizard#validate-on-button-press) I don't see any changes looking at the preview.

[tlabaj]

My question for now is, should we just remove this example or comment it out for now? Or are we comfortable exposing this as an example with the possibility of it later being deemed a demo and we just move it later?

I am OK if this example stays. We can always rework this later for the v5 release. What do you think @tlabaj ?

Also, I thought you were updating this example @jeffpuzzo (https://patternfly-react-pr-8422.surge.sh/components/wizard#validate-on-button-press) I don't see any changes looking at the preview.

I think we can leave it as is for now. We can always revisit it later.

[jpuzz0]

My question for now is, should we just remove this example or comment it out for now? Or are we comfortable exposing this as an example with the possibility of it later being deemed a demo and we just move it later?

I am OK if this example stays. We can always rework this later for the v5 release. What do you think @tlabaj ?

Also, I thought you were updating this example @jeffpuzzo (https://patternfly-react-pr-8422.surge.sh/components/wizard#validate-on-button-press) I don't see any changes looking at the preview.

@mcarrano I left the legacy example as-is and only updated the "next" rendition of "validate on button press". I can update the legacy example if we need it, but is it needed (considering it will eventually be phased out)?

[mcarrano]

@jeffpuzzo Sorry. I was looking at the wrong thing. The changes in the Next page look good and I don't feel there is a need to update the legacy examples.

[tlabaj]

LGTM