feat(WizardComposable): Created composable spinoff of wizard w/ enhancements #7703

jpuzz0 · 2022-07-15T20:46:00Z

What: Closes #7663
Depends on:

feat: Allow for 'next' and 'deprecated' react-core subpaths #7824
feat(doc-framework): Allow for 'next' core component usage alongside core components patternfly-org#3126

Note:

Unit tests for ancillary wizard components will be included in a follow-up PR.
3 starting Examples have been made under a new tab called React composable in the /wizard/components doc, and the older wizard examples still reside in a React legacy tab (mimicking the composable table doc approach).

patternfly-build · 2022-07-15T20:58:39Z

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

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

nicolethoen

Weird observation, half of the components in the side nav are missing?

Also, i think this is really great!
@thatblindgeye has just done some work on improving documentation for composable components included in this PR. It think it'd be good to include the prop table descriptions like he did for the File upload - multiple documentations, as well as the composable structure section.

nicolethoen · 2022-07-20T18:45:24Z

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

 import FinishedStep from './FinishedStep';
 import SampleForm from './SampleForm';

+If you seek a Wizard solution that allows for more composition, see the [React composition](/components/wizard/react-composition) tab.


React composable not react composition

nicolethoen · 2022-07-20T18:47:06Z

packages/react-core/src/components/WizardComposable/examples/WizardKitchenSink.tsx

+      <StepWithCustomFooter />
+    </WizardComposableStep>
+    <WizardComposableStep id="fourth-step" name="Fourth step">
+      Step 4 content


Could the 4th step include a drawer maybe?

I think since step 1 has a drawer already, it's probably not needed. What I can do though is make it a custom component (not use WizardComposableStep), which would be another form of uniqueness to add to the sink.

Oh I didn't even realize it for step 1.
So if you want to add more uniqueness to the sink, that's up to you.

jpuzz0 · 2022-07-21T15:37:42Z

Weird observation, half of the components in the side nav are missing?

Also, i think this is really great! @thatblindgeye has just done some work on improving documentation for composable components included in this PR. It think it'd be good to include the prop table descriptions like he did for the File upload - multiple documentations, as well as the composable structure section.

@nicolethoen Good catch on the left nav. I think that should be fixed now. Also added descriptions to the interfaces for those Composable prop tables & added a "Structure" section as well.

nicolethoen

LGTM

jenny-s51

Awesome work on an exciting new Wizard implementation @jeffpuzzo ! Will be a fantastic new addition to PF4!

Left a couple of suggestions to update the copy, which should help us align it more with the composable table copy! What do you think?

Otherwise LGTM!

jenny-s51 · 2022-07-25T20:31:51Z

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

+import ExternalLinkAltIcon from '@patternfly/react-icons/dist/esm/icons/external-link-alt-icon';
+import SlackHashIcon from '@patternfly/react-icons/dist/esm/icons/slack-hash-icon';
+
+PatternFly has two implementations of a `Wizard`, where the latest `WizardComposable`. Its temporary name speaks to the improvements made to the composition of the Wizard and its ansillary components.


Suggested change

PatternFly has two implementations of a `Wizard`, where the latest `WizardComposable`. Its temporary name speaks to the improvements made to the composition of the Wizard and its ansillary components.

PatternFly has two implementations of a `Wizard`.

The first is the newest `WizardComposable`. It takes a more explicit and declarative approach. Its temporary name speaks to the improvements made to the composition of the Wizard and its ancillary components.

That first sentence I had definitely didn't sound right, so thanks for this suggestion. The changes have been made.

jenny-s51 · 2022-07-25T20:33:09Z

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

+
+PatternFly has two implementations of a `Wizard`, where the latest `WizardComposable`. Its temporary name speaks to the improvements made to the composition of the Wizard and its ansillary components.
+
+The documentation for the older `Wizard` implementation can be found under the [React legacy](/components/wizard/react-legacy) tab.


Suggested change

The documentation for the older `Wizard` implementation can be found under the [React legacy](/components/wizard/react-legacy) tab.

The second is the original `Wizard` component. It is configuration-based and takes a less declarative and more implicit approach. The documentation for the older `Wizard` implementation can be found under the [React legacy](/components/wizard/react-legacy) tab.

jenny-s51

👍👍

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

thatblindgeye

Thanks for making the updates! Only have a couple more things below

thatblindgeye · 2022-07-27T17:57:33Z

packages/react-core/src/components/WizardComposable/WizardComposableBody.tsx

+    aria-label={ariaLabel}
+    aria-labelledby={ariaLabelledBy}
+    className={css(styles.wizardMain)}
+    {...(Wrapper === 'div' && { role: 'main' })}


I don't believe we should set role="main" on the component anywhere, just because it's best practice to only include one main role/element.

I think this is what inspired the idea: https://github.com/patternfly/patternfly-react/blob/main/packages/react-core/src/components/Wizard/WizardToggle.tsx#L96, but I can see where some divs might not want that role defined.

Ah interesting. So in terms of a modal-like Wizard, setting a main within the Wizard can work as long as we hide any other main elements/roles on the page. Testing the basic wizard example out a little, I added the isOpen prop to it to get a <main> element rendered in the wizard body, then ran aXe and didn't get any errors regarding any main element.

Once I removed aria-hidden="true" on the #root div, though, aXe flagged some things regarding the two main elements:

Thanks for looking into that, because I was curious as well.

thatblindgeye · 2022-07-27T17:59:25Z

packages/react-core/src/components/WizardComposable/examples/WizardBasic.tsx

+
+export const WizardBasic: React.FunctionComponent = () => (
+  <WizardComposable height={400}>
+    <WizardComposableStep name="First step" id="first-step">


This applies to all WizardComposableStep components in all three examples, but they just need to have unique IDs across examples rather than each having a step with id "first-step", etc.

Are you suggesting that in some cases we make the examples use a different naming convention for the unique ids?

yeah when you reuse id's in examples, then we have duplicate ids in the docs. so maybe the composable steps in the basic example can have ids like "basic-first-step"

tlabaj

Looking good so far Jeff! I think we want to nail down the naming of new components before taking this one in.

nicolethoen

@jeffpuzzo @tlabaj this is failing because there is still something wrong with the full page demos in this PR and the a11y tests is trying to test them. I think we might need to re-address the changes made in org for this PR to fix the full page demos

tlabaj

This is looking great Jeff! I have some nit comments.

These are things we can work towards since tis is Beta.

We also need to figure out the documentation for certain types and functions.
We will also want to build up the examples so we demonstrate functionality we had with the current wizard.

tlabaj · 2022-08-19T16:43:55Z

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

 import FinishedStep from './FinishedStep';
 import SampleForm from './SampleForm';

+If you seek a Wizard solution that allows for more composition, see the [React next](/components/wizard/react-next) tab.


nit. Wizard should be lowercase here.

packages/react-core/src/next/components/Wizard/examples/WizardKitchenSink.tsx

tlabaj · 2022-08-19T16:59:02Z

packages/react-core/src/next/components/Wizard/examples/Wizard.md

+```ts file="./WizardBasic.tsx"
+```
+
+### Custom Nav


nit. could this be updated to "Custom navigation"

tlabaj · 2022-08-19T16:59:17Z

packages/react-core/src/next/components/Wizard/examples/Wizard.md

+```ts file="./WizardCustomNav.tsx"
+```
+
+### Kitchen Sink


nit. this one should be sentence case please.

tlabaj · 2022-08-19T17:01:23Z

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

+
+/**
+ * Wrapper for all steps and hosts state, including navigation helpers, within context.
+ * The WizardContext provided by default gives any child of Wizard access to those resources.


nit. wizard her should be lowercase.

tlabaj · 2022-08-19T17:01:46Z

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

+  nav?: DefaultWizardNavProps | CustomWizardNavFunction;
+  /** The initial index the wizard is to start on (1 or higher). Defaults to 1. */
+  startIndex?: number;
+  /** Additional classes spread to the Wizard */


nit. lowercase wizard

tlabaj · 2022-08-19T17:01:58Z

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

+  height?: number | string;
+  /** Callback function when a step in the nav is clicked */
+  onNavByIndex?: WizardNavStepFunction;
+  /** Callback function after Next button is clicked */


nit. lowercase next

tlabaj · 2022-08-19T17:02:17Z

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

+  onNavByIndex?: WizardNavStepFunction;
+  /** Callback function after Next button is clicked */
+  onNext?: WizardNavStepFunction;
+  /** Callback function after Back button is clicked */


nit. lowercase back.

tlabaj · 2022-08-19T17:08:33Z

packages/react-core/src/next/components/Wizard/index.ts

+export * from './WizardToggle';
+export * from './WizardStep';
+export { useWizardFooter } from './hooks/useWizardFooter';
+export * from './types';


If we are going to export all the types her for consumption, they should be documented on the website.

tlabaj · 2022-08-19T17:17:05Z

@mcarrano @mmenestr This PR add the new composable wizard with the naming and documentation standards we had proposed. Please take a look at the React next tab and let us know if you have any concerns with naming.
This is a beta components so will be continuing to work on documentation of use cases that were supported by the current wizard.

cc @jeffpuzzo

mcarrano · 2022-08-19T17:37:13Z

I think the examples look good @tlabaj @jeffpuzzo with the assumption that we can continue to add to this.

So is "React next" the labeling we want to apply to these types of new and improved components? It deviates from what we've done elsewhere with using "Legacy." We might need more discussion on how to talk this presentation on the website, i.e. as a consumer of PatternFly do I know what "React next" means?

tlabaj · 2022-08-19T17:46:04Z

I think the examples look good @tlabaj @jeffpuzzo with the assumption that we can continue to add to this.

So is "React next" the labeling we want to apply to these types of new and improved components? It deviates from what we've done elsewhere with using "Legacy." We might need more discussion on how to talk this presentation on the website, i.e. as a consumer of PatternFly do I know what "React next" means?

@mcarrano the tab name can easily be changed if it is determined to not be meaningful enough. We did meet with Sam in terms of naming the import paths for the new components and "next" is what we proposed. That is why we decided to name the tab "React next".

…cements

…ate path alias, export as separate module

tlabaj

Looking great. The new Hooks section is a great addition. THere are still a few prop types that are not documented e.g. DefaultWizardFooterProps

tlabaj · 2022-08-22T13:31:28Z

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

+
+/** Initially inferred from WizardStep components, these properties represent what is controllable from within WizardContext for a given step. */
+
+export interface Step {


Typically if an exported type/enum/interface is directly related to a component and we do no have the intention of using it elsewhere, we prefix it with component name. This also helps avoiding collisions. Maybe rename it to WizardControllableStep?
This comment also applies to the SubStep.

I'm not exactly sure if users will need access to DefaultWizardFooterProps or other types within the types file outside of Step & SubStep, which is why the index file was updated to only expose those 2;

patternfly-react/packages/react-core/src/next/components/Wizard/index.ts

Lines 9 to 11 in 5982c3f

import * as Types from './types';

export type Step = Types.Step;

export type SubStep = Types.SubStep;

Users would need to use an import statement like the following in order to access them:
import { DefaultWizardFooterProps } from '@patternfly/react-core/src/next/components/Wizard/types';.

Is our policy that if the user can access something exported in any fashion they need to be visible in our docs?

I think If the consumer is required to use the types, it should be documented.
The reason I mention DefaultWizardFooterProps is because it is in the prop table. If something is in the prop table I think it should be defined since it is something the consumer would need to know the definition of to use it.

…xt' doc for Wizard

…gside other 'next' components, update documentation to describe types and update comments for those types in their respective locations

tlabaj · 2022-08-23T19:18:25Z

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

+  /** Callback function after back button is clicked */
+  onBack?: WizardNavStepFunction;
+  /** Callback function to save at the end of the wizard, if not specified uses onClose */
+  onSave?(): void;


For documentation purposes, can you define the functions as onSave?: () => void;. The way this is defined it is showing the prop type as void an has no default value definition

tlabaj

I just noticed that none of the Prop interfaces are listing default values in the docs. Even the props that not defaulted to undefined are blank.

jpuzz0 · 2022-08-23T19:46:38Z

I just noticed that none of the Prop interfaces are listing default values in the docs. Even the props that not defaulted to undefined are blank.

@tlabaj Should be good with the latest changes. Turns out specifying WizardStepProps vs WizardStep (as an example) does not include those defaults.

tlabaj

Looks great Jeff!

patternfly-build · 2022-08-23T20:57:26Z

Your changes have been released in:

[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
[email protected]

Thanks for your contribution! 🎉

jpuzz0 force-pushed the feature/composable-wizard branch from 1a9b7da to fc1abb7 Compare July 15, 2022 20:50

jpuzz0 force-pushed the feature/composable-wizard branch 7 times, most recently from ab0c8ac to 735f2e1 Compare July 19, 2022 19:47

nicolethoen requested a review from jenny-s51 July 20, 2022 17:27

nicolethoen requested changes Jul 20, 2022

View reviewed changes

jpuzz0 force-pushed the feature/composable-wizard branch from 6867956 to d24c010 Compare July 21, 2022 16:07

nicolethoen approved these changes Jul 21, 2022

View reviewed changes

This was referenced Jul 21, 2022

feat(Wizard - next): supporting component unit tests #7731

Merged

feat(Wizard - next): supporting component unit tests #7737

Closed

jenny-s51 suggested changes Jul 25, 2022

View reviewed changes

nicolethoen approved these changes Jul 26, 2022

View reviewed changes

jenny-s51 approved these changes Jul 27, 2022

View reviewed changes

nicolethoen requested a review from tlabaj July 27, 2022 14:15

thatblindgeye reviewed Jul 27, 2022

View reviewed changes

jpuzz0 force-pushed the feature/composable-wizard branch from e719e61 to 2e1f205 Compare July 27, 2022 17:42

thatblindgeye requested changes Jul 27, 2022

View reviewed changes

thatblindgeye approved these changes Jul 27, 2022

View reviewed changes

nicolethoen approved these changes Jul 28, 2022

View reviewed changes

tlabaj requested changes Aug 2, 2022

View reviewed changes

jpuzz0 force-pushed the feature/composable-wizard branch 2 times, most recently from c167c9b to fd2a892 Compare August 18, 2022 19:35

nicolethoen requested changes Aug 19, 2022

View reviewed changes

jpuzz0 force-pushed the feature/composable-wizard branch from fd2a892 to 6e00aff Compare August 19, 2022 15:23

tlabaj requested changes Aug 19, 2022

View reviewed changes

tlabaj added the ux review label Aug 19, 2022

tlabaj requested review from mcarrano and mmenestr August 19, 2022 17:13

jpuzz0 added 3 commits August 19, 2022 16:44

feat(WizardComposable): Created composable spinoff of wizard w/ enhan…

d6e2097

…cements

rename all composable wizard components, move to other directory, cre…

9f3ed98

…ate path alias, export as separate module

update doc framework version

1a6c9b7

jpuzz0 force-pushed the feature/composable-wizard branch from 2468984 to 08077a2 Compare August 19, 2022 20:45

add section for Hooks and address some other feedback

5982c3f

jpuzz0 force-pushed the feature/composable-wizard branch from 08077a2 to 5982c3f Compare August 19, 2022 20:51

tlabaj reviewed Aug 22, 2022

View reviewed changes

jpuzz0 force-pushed the feature/composable-wizard branch from 7a7448e to d1a70a9 Compare August 22, 2022 18:18

updating types export, including all exported interfaces in 'React ne…

78a090b

…xt' doc for Wizard

jpuzz0 force-pushed the feature/composable-wizard branch from d1a70a9 to 78a090b Compare August 22, 2022 18:20

jpuzz0 added 2 commits August 23, 2022 13:28

porting over header and nav components so they can be referenced alon…

b3ee7c3

…gside other 'next' components, update documentation to describe types and update comments for those types in their respective locations

update example comment

c231973

tlabaj reviewed Aug 23, 2022

View reviewed changes

tlabaj requested changes Aug 23, 2022

View reviewed changes

update types for documentation purposes

97bfc1c

tlabaj approved these changes Aug 23, 2022

View reviewed changes

nicolethoen approved these changes Aug 23, 2022

View reviewed changes

nicolethoen merged commit 76eab29 into patternfly:main Aug 23, 2022

-PatternFly has two implementations of a `Wizard`, where the latest `WizardComposable`. Its temporary name speaks to the improvements made to the composition of the Wizard and its ansillary components.
+PatternFly has two implementations of a `Wizard`.
+The first is the newest `WizardComposable`. It takes a more explicit and declarative approach. Its temporary name speaks to the improvements made to the composition of the Wizard and its ancillary components.


		PatternFly has two implementations of a `Wizard`, where the latest `WizardComposable`. Its temporary name speaks to the improvements made to the composition of the Wizard and its ansillary components.

		The documentation for the older `Wizard` implementation can be found under the [React legacy](/components/wizard/react-legacy) tab.

	The documentation for the older `Wizard` implementation can be found under the [React legacy](/components/wizard/react-legacy) tab.
	The second is the original `Wizard` component. It is configuration-based and takes a less declarative and more implicit approach. The documentation for the older `Wizard` implementation can be found under the [React legacy](/components/wizard/react-legacy) tab.


		/** Initially inferred from WizardStep components, these properties represent what is controllable from within WizardContext for a given step. */

		export interface Step {

	import * as Types from './types';
	export type Step = Types.Step;
	export type SubStep = Types.SubStep;

feat(WizardComposable): Created composable spinoff of wizard w/ enhancements #7703

feat(WizardComposable): Created composable spinoff of wizard w/ enhancements #7703

Uh oh!

Conversation

jpuzz0 commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolethoen 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!

jpuzz0 commented Jul 21, 2022

Uh oh!

nicolethoen left a comment

Choose a reason for hiding this comment

Uh oh!

jenny-s51 left a comment

Choose a reason for hiding this comment

Uh oh!

jenny-s51 Jul 25, 2022 • 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!

jpuzz0 Jul 26, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

jenny-s51 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thatblindgeye 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!

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

nicolethoen left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj Aug 19, 2022 • 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!

Uh oh!

tlabaj Aug 19, 2022 • 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!

jpuzz0 commented Jul 15, 2022 •

edited

Loading

patternfly-build commented Jul 15, 2022 •

edited

Loading

jenny-s51 Jul 25, 2022 •

edited

Loading

jpuzz0 Jul 26, 2022 •

edited

Loading

tlabaj Aug 19, 2022 •

edited

Loading

tlabaj Aug 19, 2022 •

edited

Loading

tlabaj Aug 19, 2022 •

edited

Loading

tlabaj Aug 19, 2022 •

edited

Loading

tlabaj Aug 19, 2022 •

edited

Loading

tlabaj Aug 19, 2022 •

edited

Loading

tlabaj Aug 19, 2022 •

edited

Loading

tlabaj commented Aug 19, 2022 •

edited

Loading

jpuzz0 commented Aug 23, 2022 •

edited

Loading