Migrate ra-ui-materialui to TypeScript (Part 1: auth) #2984
Conversation
djhi
force-pushed
the
typescript-migration-mui
branch
from
264a878 to
c82ff4b
Compare
|
|
|
The perimeter of this PR is ambitious - I'd prefer that you split the migration to avoid a large code review. As is, the perimeter of this PR is good enough - just get it working! |
| @@ -85,16 +79,12 @@ class Login extends Component { | |||
| // the componentDidMount, but if the ref doesn't exist, it will try again | |||
| // on the following componentDidUpdate. The try will be done only once. | |||
| // @see https://reactjs.org/docs/refs-and-the-dom.html#adding-a-ref-to-a-dom-element | |||
| updateBackgroundImage = (lastTry = false) => { | |||
| updateBackgroundImage = () => { | |||
There was a problem hiding this comment.
why did you remove the lastTry? See the comment above the function.
There was a problem hiding this comment.
First, because this code does not work at all, read the original carefully and look where we were passing lastTry. Second because TypeScript agrees with me
There was a problem hiding this comment.
then you should remove the comment above the method, too
- [x] auth
djhi
force-pushed
the
typescript-migration-mui
branch
from
00a4289 to
1c95a1d
Compare
djhi
changed the title
| backgroundColor: theme.palette.secondary[500], | ||
| }, | ||
| }); | ||
| interface Props extends HtmlHTMLAttributes<HTMLDivElement> { |
There was a problem hiding this comment.
we should not extend from HtmlHTMLAttributes, but use an union type in the component definition:
class LoginView extends Component<Props & WithStyles<typeof styles> & HtmlHTMLAttributes<HTMLDivElement>>
I think having the Props type extending nothing makes the intent of the component clearer.
| @@ -85,16 +79,12 @@ class Login extends Component { | |||
| // the componentDidMount, but if the ref doesn't exist, it will try again | |||
| // on the following componentDidUpdate. The try will be done only once. | |||
| // @see https://reactjs.org/docs/refs-and-the-dom.html#adding-a-ref-to-a-dom-element | |||
| updateBackgroundImage = (lastTry = false) => { | |||
| updateBackgroundImage = () => { | |||
There was a problem hiding this comment.
then you should remove the comment above the method, too
| className, | ||
| loginForm, | ||
| staticContext, | ||
| ...rest |
There was a problem hiding this comment.
Some of the props that were in sanitizeRestProps are missing here
There was a problem hiding this comment.
Because they were unnecessary. I checked
| @@ -39,7 +61,12 @@ const renderInput = ({ | |||
| const login = (auth, dispatch, { redirectTo }) => | |||
| dispatch(userLogin(auth, redirectTo)); | |||
|
|
|||
| const LoginForm = ({ classes, isLoading, handleSubmit, translate }) => ( | |||
| const LoginFormView: SFC<Props & EnhancedProps> = ({ | |||
There was a problem hiding this comment.
I think SFC is deprecated in favor of FunctionalComponent
No description provided.