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

Snackbar throws error when anchorOrigin value is number #12283

Closed
2 tasks done
nmchaves opened this issue Jul 26, 2018 · 2 comments
Closed
2 tasks done

Snackbar throws error when anchorOrigin value is number #12283

nmchaves opened this issue Jul 26, 2018 · 2 comments
Labels
bug 🐛 Something doesn't work 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.

Comments

@nmchaves
Copy link
Contributor

nmchaves commented Jul 26, 2018

  • This is a v1.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

According to the Snackbar docs docs, The <Snackbar/> component should be able to accept a number for anchorOrigin.vertical and anchorOrigin.horizontal. It should work like the <Popover/> component.

Current Behavior

MUI throws a runtime error if you pass a number:

Material-UI: capitalize(string) expects a string argument.

It's because of this line:
https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/Snackbar/Snackbar.js#L243

Steps to Reproduce

In this CodeSandbox example, it'll throw a runtime error when you try to open the snackbar.

Here is a similar (working) CodeSandbox example for a popover.

Context

I don't see this issue as a major blocker since consumers of <Snackbar/> can always customize the positioning using classes. Also, you can pass a numeric string (e.g. "10" or "10px") to anchorOrigin.vertical and anchorOrigin.horizontal.

I think the most important thing right now is that the docs and the TypeScript definitions should be consistent with the runtime behavior.

Should we update the docs and the TS definitions to indicate that numbers can't be used? If so, I'll definitely submit a PR. Or should we actually implement support for passing numbers to anchorOrigin?

@oliviertassinari oliviertassinari added component: snackbar This is the name of the generic UI component, not the React module! bug 🐛 Something doesn't work good first issue Great for first contributions. Enable to learn the contribution process. labels Jul 26, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 26, 2018

I think the most important thing right now is that the docs and the TypeScript definitions are consistent with the runtime behavior.

@nmchaves You are right, thanks for raising the issue. It's not just about the TypeScript definition that is wrong, it's also true for the prop-types, e.g.:
https://github.com/mui-org/material-ui/blob/9c671898ad6cf01281fd99b5d0d8ee11344b63db/packages/material-ui/src/Snackbar/Snackbar.js#L286

@nmchaves
Copy link
Contributor Author

@oliviertassinari Great point, thanks! This weekend, I'll update the docs, prop-types, and TypeScript definitions.

By the way, what I said here is wrong:

Also, you can pass a numeric string (e.g. "10" or "10px") to anchorOrigin.vertical and anchorOrigin.horizontal.

You can not pass a numeric string. It won't cause a runtime exception, but it won't actually position the snackbar as expected. I'll make sure that the typings prevent generic strings like "10px" from being used. We should only allow the enums.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work 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.
Projects
None yet
Development

No branches or pull requests

2 participants