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

[TypeScript] Update SnackbarContent type def to accept action prop as array #12595

Merged

Conversation

cngraf
Copy link
Contributor

@cngraf cngraf commented Aug 20, 2018

Closes #12594

This PR alters the SnackbarContentProps interface. I've updated the action prop to a type union that accepts both a single element and an array of elements. The Snackbar component already uses the same union type for its action prop (source), and I couldn't find any documentation suggesting SnackbarContent should behave any differently.

I've also added a test to confirm the existing expected behavior, outside the type defs.

@cngraf cngraf changed the title Ts allow array in snackbarcontent action [TypeScript] Update SnackbarContent type def to accept action prop as array Aug 20, 2018
@@ -3,7 +3,7 @@ import { StandardProps } from '..';
import { PaperProps } from '../Paper';

export interface SnackbarContentProps extends StandardProps<PaperProps, SnackbarContentClassKey> {
action?: React.ReactElement<any>;
action?: React.ReactElement<any> | React.ReactElement<any>[];
Copy link
Member

Choose a reason for hiding this comment

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

Have you tried React.ReactNode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the quick feedback!

I tried that just now, replacing the entire union with React.ReactNode, and it does resolve the issue. If that's the more appropriate/idiomatic fix, would it make sense to apply the same change to the Snackbar interface?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and we can update message definition too.

@oliviertassinari oliviertassinari added component: snackbar This is the name of the generic UI component, not the React module! typescript labels Aug 20, 2018
@@ -13,12 +13,12 @@ export interface SnackbarProps
React.HTMLAttributes<HTMLDivElement> & Partial<TransitionHandlerProps>,
SnackbarClassKey
> {
action?: React.ReactElement<any> | React.ReactElement<any>[];
action?: React.ReactNode;
Copy link
Member

Choose a reason for hiding this comment

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

Since this property is passed to SnackBarContent it should explicitly reference that type like SnackBarContentProps['action']. Same for message.

@oliviertassinari oliviertassinari force-pushed the ts-allow-array-in-snackbarcontent-action branch from 853c67b to 0c6397e Compare August 20, 2018 21:35
@oliviertassinari oliviertassinari merged commit 4884da8 into mui:master Aug 20, 2018
@oliviertassinari
Copy link
Member

@cngraf It's a great first pull request on Material-UI 👌🏻. Thank you for giving it a shot!

@cngraf
Copy link
Contributor Author

cngraf commented Aug 20, 2018

Thanks for making it easy to contribute, @oliviertassinari and @eps1lon! Everything is so organized and well-documented, it's approachable even for a React newcomer like myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: snackbar This is the name of the generic UI component, not the React module! typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants