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

Demo product refactor to typescript #4592

Closed
wants to merge 8 commits into from
Closed

Demo product refactor to typescript #4592

wants to merge 8 commits into from

Conversation

developerium
Copy link
Contributor

Hi Guys
I want to contribute to the refactoring demo app to typescript.
I want to refactor examples/demo/src/products folder. Please tell me if someone else is already working on it.

Stay safe

import { linkToRecord, Identifier } from 'ra-core';
import { Product } from '../types';

enum Width {
Copy link
Member

Choose a reason for hiding this comment

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

No, use the type exported by Material-ui for this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (width === 'xs') return 2;
if (width === 'sm') return 3;
if (width === 'md') return 4;
if (width === 'lg') return 5;
return 6;
};

const times = (nbChildren, fn) =>
const times = (nbChildren: number, fn: TimesCallback) =>
Copy link
Member

Choose a reason for hiding this comment

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

We really don't need a named type for the second argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

interface Props extends GridListProps {
loaded: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

No, there are way more props injected to GridList

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

loaded ? <LoadedGridList {...props} /> : <LoadingGridList {...props} />;

export default withWidth()(GridList);
export default withWidth()(GridList as any);
Copy link
Member

Choose a reason for hiding this comment

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

Not good. If you have to use any, try to look for an alternative solution. For instance, not using withWidth, but the new hooks provided by material-ui.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@developerium
Copy link
Contributor Author

I need a little help, I'm facing an issue like this:
mui/material-ui#9106

On demo/src/products/GridList.tsx:92, Link is passes in component prop of GridListTile, and also to is used with string, and we get the error TS2769: No overload matches this call.

I tried the below solutions but didn't work:
mui/material-ui#9106 (comment)
mui/material-ui#9106 (comment)

Can you direct me in the right path?
@fzaninotto @djhi

@josephktcheung
Copy link
Contributor

I need a little help, I'm facing an issue like this:
mui-org/material-ui#9106

On demo/src/products/GridList.tsx:92, Link is passes in component prop of GridListTile, and also to is used with string, and we get the error TS2769: No overload matches this call.

I tried the below solutions but didn't work:
mui-org/material-ui#9106 (comment)
mui-org/material-ui#9106 (comment)

Can you direct me in the right path?
@fzaninotto @djhi

I believe it was a bug of material-ui which should be fixed in 4.9.13 mui/material-ui@701e3ad, which allows GridListTile to accept generic component as component prop. I tried upgrading @material-ui/core to 4.9.13 but failed to compile ra-ui-materialui.

@fzaninotto would react-admin consider upgrading @material-ui/core in the near future?

@developerium
Copy link
Contributor Author

Thank you @josephktcheung for bringing this up.
I've bumped the @material-ui/core in examples/demo/package.json to latest and I can confirm that this problem was fixed, but as you mentioned, there were lot's of other new issues related to the theme and I thought that this was out of the scope of this pull request, so I want to just ignore type check on that specific line and continue with the refactoring.

@fzaninotto
Copy link
Member

Could you please rebase on next? that's where we'll add the types to the demo app.

@MohammedFaragallah
Copy link
Contributor

Hello, @developerium are you still working on this?

@developerium
Copy link
Contributor Author

Hi @MohammedFaragallah
I'm actually swamped with work and I think I'll be busy for a while, so I don't mind if anybody wants to work on this.

@fzaninotto
Copy link
Member

The demo is now entirely migrated to TypeScript.

@fzaninotto fzaninotto closed this Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants