-
Notifications
You must be signed in to change notification settings - Fork 357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Wizard - next): Enhancements & updated types #7915
feat(Wizard - next): Enhancements & updated types #7915
Conversation
Preview: https://patternfly-react-pr-7915.surge.sh A11y report: https://patternfly-react-pr-7915-a11y.surge.sh |
7ee35eb
to
9d6797e
Compare
39dec9a
to
55ac285
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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
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
Then we can make sure the color, spacing, line wrapping, etc is correct. |
2e046c6
to
f665567
Compare
@mcarrano I've addressed this comment, and it can be visually tested in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Thanks for making that update @jeffpuzzo !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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 |
@wise-king-sullyman I spoke with @nicolethoen about this who gave me some great pointers. I ended up adding a
Does this seem appropriate? (Question for both of you) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂
- Sub-steps | ||
- Step content with a drawer | ||
- Custom nav item | ||
- Disabled nav items until visited |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"navigation" here as well.
- Custom footer | ||
- Sub-steps | ||
- Step content with a drawer | ||
- Custom nav item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to "navigation" instead of the abbreviated "nav".
@@ -37,6 +40,8 @@ export interface WizardProps extends React.HTMLProps<HTMLDivElement> { | |||
width?: number | string; | |||
/** Custom height of the wizard */ | |||
height?: number | string; | |||
/** Disables nav items that haven't been visited. Defaults to false */ | |||
forceStepVisit?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typically we name our boolean props with prefixes such as "is", "are", "has".
maybe areUnvisitedStepsDisabled
. Or something that along those lines.
@@ -37,6 +40,8 @@ export interface WizardProps extends React.HTMLProps<HTMLDivElement> { | |||
width?: number | string; | |||
/** Custom height of the wizard */ | |||
height?: number | string; | |||
/** 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would update this prop name as mentioned above.
nextButtonText?: React.ReactNode; | ||
/** Custom text for the Back button */ | ||
backButtonText?: React.ReactNode; | ||
/** Custom text for the Cancel link */ | ||
cancelButtonText?: React.ReactNode; | ||
/** Optional flag to disable the first step's back button */ | ||
/** Flag to disable the next button */ | ||
disableNextButton?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe isNextButtonDisabled
disableBackButton?: boolean; | ||
/** True to hide the Cancel button */ | ||
hideCancelButton?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit isCancleButtonHidden
/** True to hide the Cancel button */ | ||
hideCancelButton?: boolean; | ||
/** True to hide the Back button */ | ||
hideBackButton?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit isBackButtonHidden
@@ -6,9 +6,9 @@ export interface WizardNavProps { | |||
/** children should be WizardNavItem components */ | |||
children?: any; | |||
/** Aria-label applied to the nav element */ | |||
'aria-label'?: string; | |||
ariaLabel?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious why you renamed these aria attributes. The way you add it before is how we have them elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with keeping them consistent, since 'aria-label' is also the name of the HTML attribute
} | ||
|
||
export const WizardFooter = ({ | ||
export const WizardFooterWrapper: React.FunctionComponent = ({ children }) => ( | ||
<footer className={css(styles.wizardFooter)}>{children}</footer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am seeing a type error here. There is no interface defined for this component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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<P = {}> {
(props: PropsWithChildren<P>, context?: any): ReactElement<any, any> | null;
propTypes?: WeakValidationMap<P> | undefined;
contextTypes?: ValidationMap<any> | undefined;
defaultProps?: Partial<P> | undefined;
displayName?: string | undefined;
}
type PropsWithChildren<P> = P & { children?: ReactNode | undefined };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. It was locally when I pulled the branch.
/** 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to an internal prop. It added when you build the step correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, that should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An aside, but I think we usually use the component
prop for this kind of thing?
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)
- Custom navigation item | ||
- Disabled navigation items until visited | ||
- 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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - punctuation
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). |
|
||
const wizardNavProps: WizardNavProps = { | ||
isExpanded: isNavExpanded, | ||
'aria-label': nav?.['aria-label'] || 'Wizard navigation', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to be 'Wizard steps' for now.
{status === WizardNavItemStatus.Error && ( | ||
<ExclamationCircleIcon color="var(--pf-global--danger-color--100)" className="pf-u-ml-sm" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to add support for this in core, then bring it back here. Created an issue for that - patternfly/patternfly#5142
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. To be clear though, is this a blocking comment or something we can add back after this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is fine to add as a follow up if @mcoker is ok with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
{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)"}} /> |
@@ -97,10 +88,10 @@ export const WizardToggle = ({ | |||
> | |||
<span className={css(styles.wizardToggleList)}> | |||
<span className={css(styles.wizardToggleListItem)}> | |||
{activeStep?.name} | |||
{currentStep?.name} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the toggleNum element here
<span className={css(styles.wizardToggleNum)}>{activeStepIndex}</span> {activeStepName} |
This is what it should look like
This is what it looks like now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I think I lost this when resolving merge conflicts in or from the previous Wizard-next PR, will add this back, good catch!
0c7c4b7
to
09b7c6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
d700058
to
dfa260f
Compare
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