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

[Slide] Wrong direction TypeScript definition #20084

Closed
wmitsuda opened this issue Mar 12, 2020 · 9 comments · Fixed by #20338
Closed

[Slide] Wrong direction TypeScript definition #20084

wmitsuda opened this issue Mar 12, 2020 · 9 comments · Fixed by #20338
Labels
bug 🐛 Something doesn't work component: transitions 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

@wmitsuda
Copy link

Please see: https://material-ui.com/api/slide/

The docs say the direction prop has a default value of "down", but if you omit it from real code, the type information will complain saying you didn't specify the direction prop.

Either the doc or the type info is wrong.

  • [ x] The issue is present in the latest release.
  • [ x] I have searched the issues of this repository and believe that this is not a duplicate.

Your Environment 🌎

Tech Version
Material-UI v4.9.5
React
Browser
TypeScript
etc.
@oliviertassinari
Copy link
Member

Please provide a full reproduction test case. This would help a lot 👷 .
A live example would be perfect. This codesandbox.io template may be a good starting point. Thank you!

@oliviertassinari oliviertassinari added the status: waiting for author Issue with insufficient information label Mar 12, 2020
@oliviertassinari
Copy link
Member

Looking at the source, it seems about right

https://github.com/mui-org/material-ui/blob/7102777b6d248e9b23740a629781726f725019c7/packages/material-ui/src/Slide/Slide.d.ts#L6

Do you want to take care of the problem? :)

@oliviertassinari oliviertassinari added component: Slide typescript good first issue Great for first contributions. Enable to learn the contribution process. and removed status: waiting for author Issue with insufficient information labels Mar 12, 2020
@oliviertassinari
Copy link
Member

@wmitsuda I believe the fix would look like this:

diff --git a/packages/material-ui/src/Slide/Slide.d.ts b/packages/material-ui/src/Slide/Slide.d.ts
index 77efeeb8d..6132d95e1 100644
--- a/packages/material-ui/src/Slide/Slide.d.ts
+++ b/packages/material-ui/src/Slide/Slide.d.ts
@@ -3,7 +3,7 @@ import { Theme } from '../styles/createMuiTheme';
 import { TransitionProps } from '../transitions/transition';

 export interface SlideProps extends TransitionProps {
-  direction: 'left' | 'right' | 'up' | 'down';
+  direction?: 'left' | 'right' | 'up' | 'down';
   ref?: React.Ref<unknown>;
   theme?: Theme;
 }

@oliviertassinari oliviertassinari changed the title Slider doc says direction prop has a default parameter [Slide] Wrong direction TypeScript definition Mar 15, 2020
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Mar 15, 2020
@belgamo
Copy link

belgamo commented Mar 16, 2020

@oliviertassinari were you able to reproduce this problem? I'm willing to take care of that, but I have not been able to reproduce it.

@oliviertassinari
Copy link
Member

I haven't tried to reproduce it. I have only looked at the source that looked wrong.

@belgamo
Copy link

belgamo commented Mar 16, 2020

🤔 @wmitsuda could you provide more details?

@igorbrasileiro
Copy link
Contributor

@belgamo if you go to this slide example at Material-UI doc and remove
direction="up", Typescript will throw an error. look here at codesandbox

What @oliviertassinari said solves the problem.

@wmitsuda I believe the fix would look like this:

@oliviertassinari oliviertassinari added component: transitions This is the name of the generic UI component, not the React module! and removed component: Slide labels Mar 24, 2020
@maksimgm
Copy link
Contributor

maksimgm commented Mar 29, 2020

I can create a PR for this issue. I'm looking to get more involved in the open source community, and love material-ui; would love to give back to the community :)

@maksimgm
Copy link
Contributor

maksimgm commented Mar 30, 2020

PR for the issue:
#20338

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: transitions 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

Successfully merging a pull request may close this issue.

5 participants