Skip to content
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

Merged
merged 5 commits into from
Jul 17, 2024

Conversation

thatblindgeye
Copy link
Contributor

@thatblindgeye thatblindgeye commented Jun 20, 2024

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:

@@ -77,7 +77,7 @@ export const Wizard = ({
onStepChange,
onSave,
onClose,
shouldFocusContent = false,
shouldFocusContent = true,
Copy link
Contributor Author

@thatblindgeye thatblindgeye Jun 20, 2024

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.

Copy link
Contributor

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

@patternfly-build
Copy link
Contributor

patternfly-build commented Jun 20, 2024

/** 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'>;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@tlabaj tlabaj merged commit d235cd8 into patternfly:v6 Jul 17, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wizard - update custom footer example to use ActionList Wizard v6 - Consider making WizardBody required
5 participants