-
Notifications
You must be signed in to change notification settings - Fork 355
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
chore(Wizard): updated API for WizardBody and docs for footer #10637
Conversation
@@ -77,7 +77,7 @@ export const Wizard = ({ | |||
onStepChange, | |||
onSave, | |||
onClose, | |||
shouldFocusContent = false, | |||
shouldFocusContent = true, |
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.
@tlabaj I believe we had said that for v6 we wanted to make this the default, rather than an opt in, correct? If that's still the case then we could remove the example that shows this functionality, as well as update failing integration tests.
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.
Yes, that is what was discussed for accessibility purposes
Preview: https://patternfly-react-pr-10637.surge.sh A11y report: https://patternfly-react-pr-10637-a11y.surge.sh |
/** Props for WizardBody that wraps content by default. Can be set to null for exclusion of WizardBody. */ | ||
body?: Omit<Omit<WizardBodyProps, 'children'>, 'children'> | null; | ||
/** Props for WizardBody that wraps content by default. */ | ||
body?: Omit<Omit<WizardBodyProps, 'children'>, 'children'>; |
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.
Should the prop also be marked as required so that undefined
can't be passed / the prop omitted? It sounds that way based on the issue, but I could be misunderstanding.
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 being optional should be fine. I'll try locally, but the logic in WizardToggle checks whether body || body === undefined
to render the WizardBody wrapping children
. We could probably just remove that conditional now,, though, since we should always want the WizardBody wrapper.
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.
Just to clarify, the body
prop in WizardStep is for passing an object of WizardBody props in; the prop name isn't exactly descriptive of the actual behavior. May be worth updating the prop to bodyProps
instead at some point.
7a2108c
to
e078f56
Compare
e078f56
to
2b63af4
Compare
What: Closes #10399 and #10319
Pairs with https://github.com/patternfly/patternfly/pull/6807/files
First commit is for the WizardBody update, second commit is for the footer example updates
Codemod: patternfly/pf-codemods#667
Additional issues: