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

SnackBarOrigin naming and optional modifier #12072

Closed
2 tasks done
kobelobster opened this issue Jul 6, 2018 · 2 comments
Closed
2 tasks done

SnackBarOrigin naming and optional modifier #12072

kobelobster opened this issue Jul 6, 2018 · 2 comments
Labels
component: snackbar This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. typescript

Comments

@kobelobster
Copy link
Contributor

kobelobster commented Jul 6, 2018

  • This is a v1.x issue (v0.x is no longer maintained).
  • I have searched the issues of this repository and believe that this is not a duplicate.

First of all, I would rename SnackBarOrigin to SnackbarOrigin, so it's in line with all other Snackbar namings.

Secondly, SnackBarOrigin is defined like this

export type SnackBarOrigin = {
  horizontal?: 'left' | 'center' | 'right' | number;
  vertical?: 'top' | 'center' | 'bottom' | number;
};

which indicates that you can define either of them, right? But when you only define horizontal for example, function capitalize(string) { will be called for both horizontal and vertical. Since vertical hasn't been defined, it's undefined, ergo not a string and the error Material-UI: capitalize(string) expects a string argument. is thrown. To fix this, you have to pass both horizontal and vertical as an option.

My suggestion (in addition to the naming) would be to make both of them mandatory, e.g. define it like this

export type SnackBarOrigin = {
  horizontal: 'left' | 'center' | 'right' | number;
  vertical: 'top' | 'center' | 'bottom' | number;
};

or don't call capitzlize for undefined strings.

Your Environment

Tech Version
Material-UI v1.0.0
@oliviertassinari oliviertassinari added typescript component: snackbar This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. labels Jul 6, 2018
@oliviertassinari
Copy link
Member

My suggestion (in addition to the naming) would be to make both of them mandatory, e.g. define it like this

@tzfrs Thanks for raising the issue. Your suggestion sounds good to me. Do you want to work on it? :)

@kobelobster
Copy link
Contributor Author

@oliviertassinari I created #12083

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! good first issue Great for first contributions. Enable to learn the contribution process. typescript
Projects
None yet
Development

No branches or pull requests

2 participants