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.

Sign up for GitHub

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

Migrate AppBar to tsx #4873

Closed
wants to merge 1 commit into from
Closed

Migrate AppBar to tsx #4873

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions ...ges/ra-ui-materialui/src/layout/AppBar.js → ...es/ra-ui-materialui/src/layout/AppBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
import React, { Children, cloneElement } from 'react';
import React, { Children, cloneElement, FC, ReactElement } from 'react';
import PropTypes from 'prop-types';
import { useDispatch } from 'react-redux';
import classNames from 'classnames';
import {
AppBar as MuiAppBar,
AppBarProps as MuiAppBarProps,
IconButton,
Toolbar,
Tooltip,
Typography,
makeStyles,
useMediaQuery,
Theme,
} from '@material-ui/core';
import MenuIcon from '@material-ui/icons/Menu';
import { toggleSidebar, useTranslate } from 'ra-core';
Expand Down Expand Up @@ -78,13 +80,12 @@ const useStyles = makeStyles(
* );
*};
*/
const AppBar = props => {
const AppBar: FC<AppBarProps> = props => {
const {
children,
classes: classesOverride,
className,
color = 'secondary',
logo,
logout,
open,
title,
Expand All @@ -93,7 +94,9 @@ const AppBar = props => {
} = props;
const classes = useStyles(props);
const dispatch = useDispatch();
const isXSmall = useMediaQuery(theme => theme.breakpoints.down('xs'));
const isXSmall = useMediaQuery<Theme>(theme =>
theme.breakpoints.down('xs')
);
const translate = useTranslate();

return (
Expand Down Expand Up @@ -147,17 +150,19 @@ const AppBar = props => {
);
};

interface Props {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @josephktcheung can you please add title property as TitleComponent from ra-core

Copy link
Contributor Author

@josephktcheung josephktcheung May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @MohammedFaragallah thanks for the review. Good catch on this, from what I understand AppBar no longer uses title prop, I guess the reason why it's not removed is because we inject title prop to AppBar in Layout. If we don't de-structure AppBar props here, we may pass a wrong title type to material-ui's AppBar by spreading the rest prop here

Therefore, I question the need of passing title prop when we cloneElement in https://github.com/marmelab/react-admin/blob/next/packages/ra-ui-materialui/src/layout/Layout.js#L124. I think title prop can be removed from AppBar which we did previously a9e4e0d but somehow reverted back by @fzaninotto 0f1b404#diff-bbb6c2d0f8bfb7588ebadf3e64ed2e57 perhaps because of the reason I mention above.

@fzaninotto please confirm if title prop is still relevant for AppBar or is it a legacy code?

logout?: ReactElement;
userMenu?: ReactElement;
open?: boolean;
}

export type AppBarProps = Props & MuiAppBarProps;

AppBar.propTypes = {
children: PropTypes.node,
classes: PropTypes.object,
className: PropTypes.string,
color: PropTypes.oneOf([
'default',
'inherit',
'primary',
'secondary',
'transparent',
]),
color: PropTypes.oneOf(['default', 'inherit', 'primary', 'secondary']),
logout: PropTypes.element,
open: PropTypes.bool,
userMenu: PropTypes.element,
Expand Down