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

fix(Wizard): adds default function to onClose prop #2863

Merged
merged 4 commits into from Sep 18, 2019

Conversation

@SpyTec
Copy link
Contributor

SpyTec commented Sep 6, 2019

What: This adds a default function to onClose so that it no longer crashes the component if left out

Additional issues: fixes #2831

cc @suomiy

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 6, 2019

PatternFly-React preview: https://patternfly-react-pr-2863.surge.sh

@suomiy

This comment has been minimized.

Copy link
Contributor

suomiy commented Sep 6, 2019

+1, but snapshots need to be updated

@SpyTec

This comment has been minimized.

Copy link
Contributor Author

SpyTec commented Sep 6, 2019

Right, thank you! - Snapshots updated

Copy link
Contributor

redallen left a comment

Fixed the nitpick for you, so approved!

@@ -121,7 +121,8 @@ export class Wizard extends React.Component<WizardProps, WizardState> {
onGoToStep: null as WizardStepFunctionType,
width: null as string,
height: null as string,
footer: null as React.ReactNode
footer: null as React.ReactNode,
onClose: () => undefined as any

This comment has been minimized.

Copy link
@tlabaj

tlabaj Sep 9, 2019

Contributor

Can we do this instead for consistency?

Suggested change
onClose: () => undefined as any
onClose: (): any => undefined

This comment has been minimized.

Copy link
@SpyTec

SpyTec Sep 10, 2019

Author Contributor

Right now there's 11 results for ): any => undefined and 22 for ) => undefined as any. Are we preferring the former when updating/writing new code?

This comment has been minimized.

Copy link
@dlabaj

dlabaj Sep 17, 2019

Contributor

We should fix these consistency issues and decide on which format we would like to use. Could add some tslint rules for things when possible. I would recommend leaving this for now and fixing some of these consistency issues in a separate issue.

@dlabaj
dlabaj approved these changes Sep 17, 2019
@@ -121,7 +121,8 @@ export class Wizard extends React.Component<WizardProps, WizardState> {
onGoToStep: null as WizardStepFunctionType,
width: null as string,
height: null as string,
footer: null as React.ReactNode
footer: null as React.ReactNode,
onClose: () => undefined as any

This comment has been minimized.

Copy link
@dlabaj

dlabaj Sep 17, 2019

Contributor

We should fix these consistency issues and decide on which format we would like to use. Could add some tslint rules for things when possible. I would recommend leaving this for now and fixing some of these consistency issues in a separate issue.

@tlabaj
tlabaj approved these changes Sep 18, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 4773f18 into patternfly:master Sep 18, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 18, 2019

Your changes have been released in:

  • @patternfly/react-core@3.104.6
  • @patternfly/react-docs@4.13.8
  • @patternfly/react-inline-edit-extension@2.11.42
  • demo-app-ts@3.0.5
  • @patternfly/react-table@2.20.22
  • @patternfly/react-topology@2.8.40
  • @patternfly/react-virtualized-extension@1.2.30

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.