convert ra-ui-materialui layout to typescript #4875

MohammedFaragallah · 2020-05-29T12:31:12Z

MohammedFaragallah · 2020-05-29T12:38:30Z

I think we should create a separate ErrorBounday component and covert layout to a functional component
some component destructure classes property out I think we can remove this pattern and typescript will warn users if they pass a non-supported property

fzaninotto · 2020-06-03T15:03:16Z

I think we should create a separate ErrorBounday component and convert Layout to a functional component

I agree, good idea!

some component destructure classes property out I think we can remove this pattern and typescript will warn users if they pass a non-supported property

Not sure I understand. Can you give me an example?

fzaninotto

Great work!

One general note: I tend to put the type definition after the component definition, to let JS-only developers read the code without being bothered by types first.

fzaninotto · 2020-06-03T15:04:29Z

packages/ra-ui-materialui/src/defaultTheme.ts

@@ -7,11 +27,6 @@ export default {
            contrastText: '#fff',
        },
    },
-    typography: {


Why did you remove this one?

I didn't mean to push that with this PR but I checked everywhere there is no use for variant title

fzaninotto · 2020-06-03T15:05:15Z

packages/ra-ui-materialui/src/layout/CardContentInner.tsx

-const CardContentInner = props => {
-    const { className, children } = props;
+const CardContentInner: FC<CardContentProps> = props => {
+    const { className, children, ...rest } = props;


Not necessary for now, may have side effects... Please revert this change.

fzaninotto · 2020-06-03T15:13:46Z

packages/ra-ui-materialui/src/layout/Error.tsx

@@ -52,7 +52,15 @@ function goBack() {
    window.history.go(-1);
 }

-const Error = props => {
+export interface ErrorComponentProps {


ErrocComponentProps => ErrorProps (we never include the name "component" in other interface names)

fzaninotto · 2020-06-03T15:17:44Z

packages/ra-ui-materialui/src/layout/Error.tsx

@@ -86,12 +94,8 @@ const Error = props => {
                    </ExpansionPanel>
                )}
                <div className={classes.toolbar}>
-                    <Button
-                        variant="contained"
-                        icon={<History />}


I think startIcon is the right prop to get the alignment icon/text

fzaninotto · 2020-06-03T15:19:38Z

packages/ra-ui-materialui/src/layout/Layout.tsx

+    notification?: ComponentType<NotificationProps>;
+    open?: boolean;
+    sidebar?: ComponentType<DrawerProps>;
+    title: TitleComponent;


It's optional at build time (but required at runtime).

fzaninotto · 2020-06-03T15:27:41Z

packages/ra-ui-materialui/src/layout/NotFound.tsx

@@ -62,8 +69,8 @@ const NotFound = props => {
                <div>{translate('ra.message.not_found')}.</div>
            </div>
            <div className={classes.toolbar}>
-                <Button variant="contained" icon={<History />} onClick={goBack}>


use startIcon

fzaninotto · 2020-06-03T15:29:07Z

packages/ra-ui-materialui/src/layout/Sidebar.tsx

-        classes: classesOverride,
-        ...rest
-    } = props;
+interface Props extends DrawerProps {


let's call it SidebarProps and export it.

fzaninotto · 2020-06-03T15:29:46Z

packages/ra-ui-materialui/src/layout/Sidebar.tsx

    const handleClose = () => dispatch(setSidebarVisibility(false));
    const toggleSidebar = () => dispatch(setSidebarVisibility(!open));
-    const classes = useStyles({ ...props, open });
+    const classes = useStyles({ open });


not good, we must pass the complete props object or this will cause rerenderings

fzaninotto · 2020-06-03T15:30:51Z

packages/ra-ui-materialui/src/layout/Sidebar.tsx

-        ...rest
-    } = props;
+interface Props extends DrawerProps {
+    children: ReactElement;


I think it should be React.ReactNode

ReactNode gives an error

Type 'string' is not assignable to type 'ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<any, any, any>)>) | (new (props: any) => Component<any, any, any>)>'.

fzaninotto · 2020-06-03T15:32:26Z

packages/ra-ui-materialui/src/layout/TitleForRecord.tsx

+}
+
+const TitleForRecord: FC<Props> = props =>
+    props.record ? <Title {...props} /> : null;


the previous version white listed the props it passed to the <Title> component. Please keep that behavior.

fzaninotto · 2020-06-05T07:37:21Z

packages/ra-ui-materialui/src/layout/ErrorBoundary.tsx

+
+import { ErrorProps } from './Error';
+
+class _ErrorBoundary extends Component<


We don't use the _ prefix for private classes in react-admin. I'd call the two components ErrorBoundary and ErrorBoundaryWithRoute.

fzaninotto · 2020-06-05T13:41:45Z

packages/ra-ui-materialui/src/layout/Title.tsx

@@ -26,6 +32,13 @@ const Title = ({ className, defaultTitle, locale, record, title, ...rest }) => {
    return createPortal(titleElement, container);
 };

+export interface TitleProps {
+    defaultTitle: TitleComponent;


it's optional. Either one of title or defaultTitle should be defined.

fzaninotto · 2020-06-05T13:42:38Z

packages/ra-ui-materialui/src/layout/TitleForRecord.tsx

+    ) : null;
+
+interface Props extends TitleProps {
+    record: TitleProps['record'];


I don't understand this type. It specializes nothing.

to make record required, but to make things more clear I can add a helper type?

export type SomeRequired<T, U extends keyof T> = Omit<T, U> & Required<Pick<T, U>>;

+ interface Props extends <TitleProps,'record'> { - interface Props extends TitleProps { - record: TitleProps['record'];

fzaninotto · 2020-06-05T13:43:34Z

packages/ra-ui-materialui/src/layout/NotFound.tsx

+
+export interface NotFoundProps extends RouteComponentProps {
+    className: string;
+    title: TitleProps['defaultTitle'];


Optional I think

MohammedFaragallah · 2020-06-08T09:16:39Z

@fzaninotto closed by milestone release?

WiXSL · 2020-07-15T15:11:58Z

Is this PR still valid?

Luwangel · 2020-07-16T09:37:18Z

@WiXSL I don't know. Let's ask François, he'll be back soon 😉

@fzaninotto I'm not sure I understand why you closed that PR.

convert ra-ui-materialui layout to typescript

bcf850e

fix failling checks

11ac651

fzaninotto requested changes Jun 3, 2020

View reviewed changes

MohammedFaragallah added 5 commits June 4, 2020 10:28

revert CardContentInner changes

cf8e9e6

startIcon prop for before Button children icon

fab3749

move type definitions after components

4e2532f

fix review requested changes

e244a9c

sepereate ErrorBoundary from Layout

726c377

MohammedFaragallah requested a review from fzaninotto June 4, 2020 12:17

fzaninotto requested changes Jun 5, 2020

View reviewed changes

fzaninotto closed this Jun 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert ra-ui-materialui layout to typescript #4875

convert ra-ui-materialui layout to typescript #4875

MohammedFaragallah commented May 29, 2020

MohammedFaragallah commented May 29, 2020 •

edited

Loading

fzaninotto commented Jun 3, 2020

fzaninotto left a comment

fzaninotto Jun 3, 2020

MohammedFaragallah Jun 4, 2020

fzaninotto Jun 3, 2020

fzaninotto Jun 3, 2020

fzaninotto Jun 3, 2020

fzaninotto Jun 3, 2020

fzaninotto Jun 3, 2020

fzaninotto Jun 3, 2020

fzaninotto Jun 3, 2020

fzaninotto Jun 3, 2020

MohammedFaragallah Jun 4, 2020

fzaninotto Jun 3, 2020

fzaninotto Jun 5, 2020

fzaninotto Jun 5, 2020

fzaninotto Jun 5, 2020

MohammedFaragallah Jun 5, 2020

fzaninotto Jun 5, 2020

MohammedFaragallah commented Jun 8, 2020

WiXSL commented Jul 15, 2020

Luwangel commented Jul 16, 2020


		import { ErrorProps } from './Error';

		class _ErrorBoundary extends Component<

convert ra-ui-materialui layout to typescript #4875

convert ra-ui-materialui layout to typescript #4875

Conversation

MohammedFaragallah commented May 29, 2020

MohammedFaragallah commented May 29, 2020 • edited Loading

fzaninotto commented Jun 3, 2020

fzaninotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohammedFaragallah commented Jun 8, 2020

WiXSL commented Jul 15, 2020

Luwangel commented Jul 16, 2020

MohammedFaragallah commented May 29, 2020 •

edited

Loading