Skip to content

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

Merged
tlabaj merged 2 commits intopatternfly:mainfrom
jpuzz0:feature/composable-wizard-more-tests
Sep 19, 2022
Merged

feat(Wizard - next): supporting component unit tests#7731
tlabaj merged 2 commits intopatternfly:mainfrom
jpuzz0:feature/composable-wizard-more-tests

Conversation

@jpuzz0
Copy link
Contributor

@jpuzz0 jpuzz0 commented Jul 21, 2022 •
edited
Loading

What: Closes #7737

@jpuzz0 jpuzz0 added the testing label Jul 21, 2022
@jpuzz0 jpuzz0 marked this pull request as draft July 21, 2022 19:52
@patternfly-build
Copy link
Collaborator

patternfly-build commented Jul 21, 2022 •
edited
Loading

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

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

@jpuzz0 jpuzz0 removed the testing label Jul 25, 2022
@jpuzz0 jpuzz0 force-pushed the feature/composable-wizard-more-tests branch from 057f828 to 8eb62e9 Compare August 24, 2022 00:21
@jpuzz0 jpuzz0 marked this pull request as ready for review August 24, 2022 00:21
@jpuzz0 jpuzz0 changed the title feat(WizardComposable): supporting component unit tests feat(Wizard): supporting component unit tests Aug 24, 2022
@jpuzz0 jpuzz0 force-pushed the feature/composable-wizard-more-tests branch from 8eb62e9 to 8f07d76 Compare August 24, 2022 00:50
@jpuzz0 jpuzz0 changed the title feat(Wizard): supporting component unit tests feat(Wizard - next): supporting component unit tests Sep 1, 2022
@jpuzz0 jpuzz0 mentioned this pull request Sep 1, 2022
Closed
@jpuzz0 jpuzz0 force-pushed the feature/composable-wizard-more-tests branch from 8f07d76 to 3eb5f6c Compare September 1, 2022 13:51
@jpuzz0 jpuzz0 mentioned this pull request Sep 1, 2022
Merged
nicolethoen
nicolethoen reviewed Sep 2, 2022
View reviewed changes
packages/react-core/src/next/components/Wizard/WizardToggle.tsx Outdated
goToStepByIndex: (index: number) => void;
/** The button's aria-label */
/** Custom WizardNav or callback used to create a default WizardNav */
nav?: DefaultWizardNavProps | CustomWizardNavFunction;
Copy link
Contributor

@nicolethoen nicolethoen Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible/wise to have the nav and the footer both use the same pattern? Right now, the custom footer is a props object or a react node and the nav is a props object or a function.

Copy link
Contributor Author

@jpuzz0 jpuzz0 Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary, but we could make a CustomWizardFooterFunction if you want that returns onNext, onBack, and onClose. Thoughts on that change?

Copy link
Contributor

@nicolethoen nicolethoen Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'd be helpful if people want a custom footer with custom buttons, right? so maybe that'd be wise

Copy link
Contributor Author

@jpuzz0 jpuzz0 Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. That will make things complicated with how the footer is managed in context. We'd end up initializing footer's state variable as a function, which I'm not sure is even possible. Maybe I can try to do this in the next PR here where I'm updating a bunch of types (and including a new one called CustomWizardNavItemFunction - very similar to CustomWizardNavFunction):
https://github.com/patternfly/patternfly-react/pull/7915/files#diff-087f56845137e426d26fe8b189b155902a59de75ee7e2beff5c07db4d4f9c25f

Unless you think this is a blocking change for this PR.

Copy link
Contributor

@nicolethoen nicolethoen Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's blocking since all this is beta :)

Copy link
Contributor Author

@jpuzz0 jpuzz0 Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool. I will address it in that other PR then, thanks!

wise-king-sullyman
wise-king-sullyman reviewed Sep 2, 2022
View reviewed changes
Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I won't have time to review this in depth until I return from PTO on Friday 9/9, but one thing that I know will need to change is that the userEvent usage will need to be updated to be compatible with user-event v14+

@jpuzz0 jpuzz0 force-pushed the feature/composable-wizard-more-tests branch from 3eb5f6c to 0007e01 Compare September 6, 2022 14:22
tlabaj
tlabaj approved these changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jpuzz0
Copy link
Contributor Author

jpuzz0 commented Sep 8, 2022

Unfortunately I won't have time to review this in depth until I return from PTO on Friday 9/9, but one thing that I know will need to change is that the userEvent usage will need to be updated to be compatible with user-event v14+

This has been addressed.

wise-king-sullyman
wise-king-sullyman reviewed Sep 9, 2022
View reviewed changes
Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a couple of nits and a couple of questions about places where I think it would be better to test behavior on a sub-component directly.

wise-king-sullyman
wise-king-sullyman approved these changes Sep 14, 2022
View reviewed changes
Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@jpuzz0 jpuzz0 force-pushed the feature/composable-wizard-more-tests branch from 20a54bd to b36baeb Compare September 19, 2022 14:51
@nicolethoen nicolethoen requested a review from tlabaj September 19, 2022 15:24
@tlabaj tlabaj merged commit 2e4ec26 into patternfly:main Sep 19, 2022
@patternfly-build
Copy link
Collaborator

patternfly-build commented Sep 19, 2022

Your changes have been released in:

  • eslint-plugin-patternfly-react@4.80.0
  • @patternfly/react-catalog-view-extension@4.92.0
  • @patternfly/react-charts@6.94.0
  • @patternfly/react-code-editor@4.82.0
  • @patternfly/react-console@4.92.0
  • @patternfly/react-core@4.241.0
  • @patternfly/react-docs@5.102.0
  • @patternfly/react-icons@4.92.0
  • @patternfly/react-inline-edit-extension@4.86.0
  • demo-app-ts@4.201.0
  • @patternfly/react-integration@4.203.0
  • @patternfly/react-log-viewer@4.86.0
  • @patternfly/react-styles@4.91.0
  • @patternfly/react-table@4.110.0
  • @patternfly/react-tokens@4.93.0
  • @patternfly/react-topology@4.88.0
  • @patternfly/react-virtualized-extension@4.88.0
  • transformer-cjs-imports@4.79.0

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wise-king-sullyman wise-king-sullyman wise-king-sullyman approved these changes

@nicolethoen nicolethoen nicolethoen approved these changes

@tlabaj tlabaj Awaiting requested review from tlabaj

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat(Wizard - next): supporting component unit tests

5 participants

@jpuzz0 @patternfly-build @wise-king-sullyman @nicolethoen @tlabaj