feat(Wizard - next): Enhancements & updated types by jpuzz0 · Pull Request #7915 · patternfly/patternfly-react

jpuzz0 · 2022-09-01T14:01:38Z

What: Closes #7914

Might not want to review until unit tests are reviewed and merged here: #7731
This commit diff is what's new to this PR: 8a04839

patternfly-build · 2022-09-01T14:12:47Z

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

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

mcarrano

@jeffpuzzo this looks good with one exception. After discussing with the design team, we feel like the status icon that annotates a step in the menu should fall immediately after the item label, not right aligned like you have it. This would be similar to the way we place the help (?) on form elements. @mcoker should be able to provide input on standard spacing. It may be that we should have a core implementation? I can create a mockup, if necessary.

mcoker · 2022-09-14T14:34:57Z

So that it will wrap with the text, it's probably best just to put it inline with the step content and apply a margin. Here I've used the sm margin, which is about 8px.

It may be that we should have a core implementation?

It looks like it should since it looks like the icon is created by our component based on a step status -

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

Lines 105 to 108 in 55ac285

    
           <Flex justifyContent={{ default: 'justifyContentSpaceBetween' }}> 
        
             {content} 
        
             {status === 'error' && <ExclamationCircleIcon color="var(--pf-global--danger-color--100)" />} 
        
           </Flex>

Then we can make sure the color, spacing, line wrapping, etc is correct.

jpuzz0 · 2022-09-14T20:56:52Z

Thanks @mcarrano @mcoker for the feedback. I'll be making this change as soon as the PR this is branched off of is merged and am able to rebase/resolve some anticipated conflicts.

jpuzz0 · 2022-09-19T15:51:45Z

@jeffpuzzo this looks good with one exception. After discussing with the design team, we feel like the status icon that annotates a step in the menu should fall immediately after the item label, not right aligned like you have it. This would be similar to the way we place the help (?) on form elements. @mcoker should be able to provide input on standard spacing. It may be that we should have a core implementation? I can create a mockup, if necessary.

@mcarrano I've addressed this comment, and it can be visually tested in the Kitchen sink example when the PR surge link is available. Please let me know if there's anything else that is of concern that I should address in this PR.

mcarrano

This looks great. Thanks for making that update @jeffpuzzo !

wise-king-sullyman

This looks pretty solid, one bug: hitting back while on the first step of the kitchen sink example causes a type error

Also I think it would maybe be good for there to be some automatic a11y behavior (like a generated label if the consumer doesn't provide one) for Wizard steps that have an error status, since it seems that at the moment that status isn't communicated in an accessible way. Thoughts?

jpuzz0 · 2022-09-23T06:38:14Z

automatic a11y behavior

The Back button was meant to be disabled for Step 1, which I'll fix now. Since that example uses a custom footer, that had to be done manually.

As for any labels for a11y having to do with the status of a Wizard step, I could conditionally add an aria-label to the nav item or maybe the error icon itself? I might defer to others on the team for best practices in that regard.

jpuzz0 · 2022-09-23T15:49:00Z

As for any labels for a11y having to do with the status of a Wizard step, I could conditionally add an aria-label to the nav item or maybe the error icon itself? I might defer to others on the team for best practices in that regard.

@wise-king-sullyman I spoke with @nicolethoen about this who gave me some great pointers. I ended up adding a visited as well to show up conditionally in an aria-label, so the breakdown with a step label of Step label would be:

If not visited and without an 'error' status, do not add an aria-label to the button
If visited and not current, the screen reads Step label, visited
If status === 'error', the screen reads Step label, error
If visited, not current and status === 'error', the screen reads Step label, error, visited

Does this seem appropriate? (Question for both of you)

wise-king-sullyman

Looks great to me! The only other thing I can think of that might be nice would be some usage of aria-live so that screen reader users are made aware of the content changes that occur.

I would be fine with that being done under a different issue though, so I'm going ahead and approving 🙂

tlabaj · 2022-09-23T12:48:35Z

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

+- Sub-steps
+- Step content with a drawer
+- Custom nav item
+- Disabled nav items until visited


"navigation" here as well.

tlabaj · 2022-09-23T12:48:53Z

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

+- Custom footer
+- Sub-steps
+- Step content with a drawer
+- Custom nav item


Can we change this to "navigation" instead of the abbreviated "nav".

tlabaj · 2022-09-23T12:55:45Z

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

  /** Custom height of the wizard */
  height?: number | string;
+  /** Disables nav items that haven't been visited. Defaults to false */
+  forceStepVisit?: boolean;


typically we name our boolean props with prefixes such as "is", "are", "has".
maybe areUnvisitedStepsDisabled. Or something that along those lines.

tlabaj · 2022-09-23T12:57:08Z

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

+  /** Disables nav items that haven't been visited. Defaults to false */
+  forceStepVisit?: boolean;
  /** Flag to unmount inactive steps instead of hiding. Defaults to true */
  unmountInactiveSteps?: boolean;


I would update this prop name as mentioned above.

tlabaj · 2022-09-23T12:59:46Z

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

  cancelButtonText?: React.ReactNode;
-  /** Optional flag to disable the first step's back button */
+  /** Flag to disable the next button */
+  disableNextButton?: boolean;


Maybe isNextButtonDisabled

tlabaj · 2022-09-23T13:00:41Z

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

+  /** Flag to disable the back button */
  disableBackButton?: boolean;
+  /** True to hide the Cancel button */
+  hideCancelButton?: boolean;


nit isCancleButtonHidden

tlabaj · 2022-09-23T13:01:00Z

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

+  /** True to hide the Cancel button */
+  hideCancelButton?: boolean;
+  /** True to hide the Back button */
+  hideBackButton?: boolean;


nit isBackButtonHidden

tlabaj · 2022-09-23T13:02:40Z

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

  children?: any;
  /** Aria-label applied to the nav element */
-  'aria-label'?: string;
+  ariaLabel?: string;


Curious why you renamed these aria attributes. The way you add it before is how we have them elsewhere?

No reason outside of not wanting to access them in the internal wizard's navigation via bracket notation nav['aria-label'] vs nav.ariaLabel. I can revert since you're right in that elsewhere they are written as hyphenated and probably better to be consistent.

I agree with keeping them consistent, since 'aria-label' is also the name of the HTML attribute

tlabaj · 2022-09-23T16:16:23Z

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


-export const WizardFooter = ({
+export const WizardFooterWrapper: React.FunctionComponent = ({ children }) => (
+  <footer className={css(styles.wizardFooter)}>{children}</footer>


I am seeing a type error here. There is no interface defined for this component.

Are you seeing a type error locally? FunctionComponent has an interface with optional children, which is why there should be no issue here;

interface FunctionComponent { (props: PropsWithChildren, context?: any): ReactElement<any, any> | null; propTypes?: WeakValidationMap | undefined; contextTypes?: ValidationMap<any> | undefined; defaultProps?: Partial | undefined; displayName?: string | undefined; } type PropsWithChildren = P & { children?: ReactNode | undefined };

Yup. It was locally when I pulled the branch.

tlabaj · 2022-09-23T16:21:18Z

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

+  /** Flag to determine whether the step is hidden */
+  isHidden?: boolean;
+  /** Content shown when the step's nav item is selected. When treated as a parent step, only sub-step content will be shown. */
+  component?: React.ReactElement;


Is this meant to an internal prop. It added when you build the step correct?

Good catch, that should be removed.

tlabaj

LGTM!

mcoker

An aside, but I think we usually use the component prop for this kind of thing?

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

Line 19 in 9b9a020

wrapperElement?: React.ElementType;

The main issue I see is that we'll want to have the step number visible in the nav toggle on small screens. And we'll want to have status support on the step in core - in this PR it's done by hard-coding the danger color on the SVG and using a utility class to create space between the SVG and the step text (utility classes aren't included with PF by default - you have to import those separately)

mcoker · 2022-10-04T19:23:12Z

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

+- Action to toggle visibility of a step
+- Action to toggle navigation item error status

 Custom operations when navigating between steps can be achieved by utilizing `onNext`, `onBack` or `onNavByIndex` properties whose callback functions return the 'id' and 'name' of the currently focused step (currentStep), and the previously focused step (previousStep).


nit - punctuation

Suggested change

Custom operations when navigating between steps can be achieved by utilizing `onNext`, `onBack` or `onNavByIndex` properties whose callback functions return the 'id' and 'name' of the currently focused step (currentStep), and the previously focused step (previousStep).

Custom operations when navigating between steps can be achieved by utilizing `onNext`, `onBack`, or `onNavByIndex` properties whose callback functions return the 'id' and 'name' of the currently focused step (currentStep) and the previously focused step (previousStep).

mcoker · 2022-10-04T21:46:15Z

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

+
+  const wizardNavProps: WizardNavProps = {
+    isExpanded: isNavExpanded,
+    'aria-label': nav?.['aria-label'] || 'Wizard navigation',


I'm not sure what the default label should be, but fwiw the rotor menu will read this as "wizard navigation navigation" (because it's a <nav> element). For our page navigation, we use "Global" as the label for the main nav (either vertical or horizontal) and "Local" for tertiary nav and horizontal sub-nav. The existing wizard nav aria-label is "steps" in core, and "[example name] steps" in react. I wonder if it's best to stick with that?

Updated to be 'Wizard steps' for now.

mcoker · 2022-10-04T21:54:25Z

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

+            {status === WizardNavItemStatus.Error && (
+              <ExclamationCircleIcon color="var(--pf-global--danger-color--100)" className="pf-u-ml-sm" />


We'll want to add support for this in core, then bring it back here. Created an issue for that - patternfly/patternfly#5142

Agreed. To be clear though, is this a blocking comment or something we can add back after this PR?

I think it is fine to add as a follow up if @mcoker is ok with it.

Fine with me, though it's worth noting that the utility class will not work unless the utility class CSS has been imported either in the component or into the user's app. Assuming we want it to work in the interim, it could be rewritten as...

Suggested change

{status === WizardNavItemStatus.Error && (

<ExclamationCircleIcon color="var(--pf-global--danger-color--100)" className="pf-u-ml-sm" />

{status === WizardNavItemStatus.Error && (

<ExclamationCircleIcon color="var(--pf-global--danger-color--100)" style={{marginLeft: "var(--pf-global--spacer--sm)"}} />

mcoker · 2022-10-04T22:03:25Z

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

        <span className={css(styles.wizardToggleList)}>
          <span className={css(styles.wizardToggleListItem)}>
-            {activeStep?.name}
+            {currentStep?.name}


Missing the toggleNum element here

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

Line 83 in af40c12

{activeStepIndex} {activeStepName}

This is what it should look like

This is what it looks like now

Ah I think I lost this when resolving merge conflicts in or from the previous Wizard-next PR, will add this back, good catch!

mcoker

LGTM!

jpuzz0 force-pushed the feature/wizard-next-enhancements branch from 7ee35eb to 9d6797e Compare September 1, 2022 14:19

tlabaj requested a review from mcarrano September 1, 2022 16:14

tlabaj added the ux review label Sep 1, 2022

jpuzz0 force-pushed the feature/wizard-next-enhancements branch 2 times, most recently from 39dec9a to 55ac285 Compare September 6, 2022 13:57

mcarrano suggested changes Sep 8, 2022

View reviewed changes

jpuzz0 changed the title ~~feat(Wizard - next): Add more enhancements~~ feat(Wizard - next): Enhancements & updated types Sep 15, 2022

jpuzz0 force-pushed the feature/wizard-next-enhancements branch 5 times, most recently from 2e046c6 to f665567 Compare September 19, 2022 15:49

mcarrano approved these changes Sep 19, 2022

View reviewed changes

tlabaj added the css review label Sep 22, 2022

tlabaj requested a review from mcoker September 22, 2022 17:22

nicolethoen requested review from tlabaj and wise-king-sullyman September 22, 2022 18:01

wise-king-sullyman reviewed Sep 22, 2022

View reviewed changes

wise-king-sullyman approved these changes Sep 23, 2022

View reviewed changes

tlabaj requested changes Sep 23, 2022

View reviewed changes

tlabaj approved these changes Sep 23, 2022

View reviewed changes

nicolethoen approved these changes Sep 23, 2022

View reviewed changes

mcoker mentioned this pull request Oct 4, 2022

Wizard - support for step status patternfly/patternfly#5142

Closed

mcoker reviewed Oct 4, 2022

View reviewed changes

jpuzz0 force-pushed the feature/wizard-next-enhancements branch from 0c7c4b7 to 09b7c6d Compare October 5, 2022 14:35

mcoker approved these changes Oct 5, 2022

View reviewed changes

feat(Wizard - next): Enhancements & updated types

dfa260f

jpuzz0 force-pushed the feature/wizard-next-enhancements branch from d700058 to dfa260f Compare October 5, 2022 16:05

tlabaj merged commit 04f5004 into patternfly:main Oct 5, 2022

	Custom operations when navigating between steps can be achieved by utilizing `onNext`, `onBack` or `onNavByIndex` properties whose callback functions return the 'id' and 'name' of the currently focused step (currentStep), and the previously focused step (previousStep).
	Custom operations when navigating between steps can be achieved by utilizing `onNext`, `onBack`, or `onNavByIndex` properties whose callback functions return the 'id' and 'name' of the currently focused step (currentStep) and the previously focused step (previousStep).

		{status === WizardNavItemStatus.Error && (
		<ExclamationCircleIcon color="var(--pf-global--danger-color--100)" className="pf-u-ml-sm" />

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

mcoker commented Sep 14, 2022

Uh oh!

jpuzz0 commented Sep 14, 2022

Uh oh!

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

Uh oh!

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

jpuzz0 commented Sep 23, 2022

Uh oh!

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

Uh oh!

Uh oh!

wise-king-sullyman 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!

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!

mcoker 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

jpuzz0 commented Sep 1, 2022 •

edited

Loading

patternfly-build commented Sep 1, 2022 •

edited

Loading

jpuzz0 commented Sep 19, 2022 •

edited

Loading

jpuzz0 commented Sep 23, 2022 •

edited

Loading