1
\$\begingroup\$

I am trying to optimize my navigation drawer code. I have tried and followed the BEM naming convention for my CSS although I would like it reviewed with respect to the best practices being followed. Also, I have used javascript arrow functions for my list in order to avoid repetition of the list item which also needs to be reviewed since I have a nested list where I have used a workaround.

Please find below my code for the navigation drawer list:-

import React from 'react';
import PropTypes from 'prop-types';
import { withStyles, Button } from '@material-ui/core';
import ListSubheader from '@material-ui/core/ListSubheader';
import List from '@material-ui/core/List';
import ListItem from '@material-ui/core/ListItem';
import Collapse from '@material-ui/core/Collapse';
import ExpandLess from '@material-ui/icons/ExpandLess';
import ExpandMore from '@material-ui/icons/ExpandMore';
import MenuIcon from '@material-ui/icons/ArrowForward';
import IconButton from '@material-ui/core/IconButton';
import Divider from '@material-ui/core/Divider';
import Typography from '@material-ui/core/Typography';
import Link from 'next/link';

const fbIcon = '../static/facebook-icon.png';
const twitterIcon = '../static/twitter-icon.png';
const linkedinIcon = '../static/linkedin-icon.png';
const androidIcon = '../static/androidIcon.png';
const iosIcon = '../static/iosIcon.png';

const styles = theme => ({
  'list-container': {
    width: '100%',
    maxWidth: 360,
    backgroundColor: '#2ec3d2',
    [theme.breakpoints.down('sm')]: {
      maxWidth: '15rem',
    }
  },
  'list-container__subheader': {
    display: 'flex',
    flexDirection: 'column',
    backgroundColor: '#2ec3d2'
  },
  'list-container__nested': {
    paddingLeft: theme.spacing.unit * 4
  },
  'list-container__button': {
    color: theme.palette.getContrastText('#fff'),
    backgroundColor: '#fff',
    marginBottom: 20,
  },
  'list-container__arrowicon': {
    width: 50,
    height: 50,
    color: '#fff'
  },
  'list-container__arrow-container': {
    display: 'flex',
    justifyContent: 'flex-end',
    marginTop: 31,
    marginBottom: 22,
    [theme.breakpoints.down('sm')]: {
      marginTop: 11,
      marginBottom: 11
    }
  },
  'list-container__row': {
    display: 'flex',
    flexDirection: 'row',
    alignItems: 'flex-end'
  },
  'list-container__expicon': {
    marginLeft: 20,
    color: '#fff'
  },
  'list-container__headertxt': {
    fontSize: '14px',
    fontFamily: 'Montserrat,sans-serif',
    lineHeight: 1,
    textAlign: 'left',
    color: '#fff',
    textTransform: 'uppercase'
  },
  'social-container': {
    display: 'flex',
    flexDirection: 'row',
    justifyContent: 'space-around',
    paddingLeft: 10,
    marginTop: 10
  },
  'social-container__icon': {
    width: 30,
    height: 30
  },
  'social-container__psicon': {
    width: '145px',
    height: '45px',
    display: 'flex',
    alignItems: 'left',
    marginBottom: '5%',
    marginTop: '8%',
    [theme.breakpoints.down('sm')]: {
      marginTop: '10%',
      width: '104px',
      height: '32px'
    },
    [theme.breakpoints.down('xs')]: {
      marginTop: '10%',
      width: '94px',
      height: '30px'
    }

  }
});

const getNavigationLinks = (navigationObj, classes) => Object.keys(navigationObj)
  .map(linkName => (
    <Link key={linkName} href={navigationObj[linkName]}>
      {/* eslint-disable-next-line jsx-a11y/anchor-is-valid */}
      <ListItem button>
        <Typography className={classes['list-container__headertxt']} variant="subtitle2" gutterBottom>
          {linkName}
        </Typography>
      </ListItem>

    </Link>
  ));

const getNestedLinks = (navigationObj, classes) => Object.keys(navigationObj)
  .map(linkName => (
    <Link key={linkName} href={navigationObj[linkName]}>
      {/* eslint-disable-next-line jsx-a11y/anchor-is-valid */}
      <ListItem button className={classes['list-container__nested']}>
        <Typography className={classes['list-container__headertxt']} variant="subtitle2" gutterBottom>
          {linkName}
        </Typography>
      </ListItem>

    </Link>
  ));

class NavigationList extends React.Component {
    state = {
      open: false
    };

    handleClick = () => {
      this.setState(state => ({ open: !state.open }));
    };

    render() {
      const { classes } = this.props;
      const { open } = this.state;
      return (
        <div className={classes['list-container']}>
          <List
            component="nav"
            subheader={(
              <ListSubheader
                className={classes['list-container__subheader']}
                component="div"
              >
                <div className={classes['list-container__arrow-container']}>
                  <IconButton className={classes['list-container__arrowicon']} aria-label="arrow">
                    <MenuIcon />
                  </IconButton>
                </div>
                <Button
                  className={classes['list-container__button']}
                  variant="contained"
                  size="large"
                  style={{
                    color: '#2ec3d2'
                  }}
                >
                Download
                </Button>
              </ListSubheader>
              )}
          >
            {getNavigationLinks(
              {
                HOME: '/',
                'EXISTING CUSTOMER LOGIN': 'https://somelink/',
                'ABOUT Us': '/aboutus',
                CAREERS: '/careers',
              },
              classes
            )}
            <ListItem button onClick={this.handleClick}>
              <div className={classes['list-container__row']}>
                <Typography className={classes['list-container__headertxt']} variant="subtitle2" gutterBottom>
                PRODUCTS
                </Typography>
                {open ? <ExpandLess className={classes['list-container__expicon']} />
                  : <ExpandMore className={classes['list-container__expicon']} />}
              </div>
            </ListItem>
            <Collapse in={open} timeout="auto" unmountOnExit>
              <List component="div" disablePadding>
                {getNestedLinks(
                  {
                    'INSTANT CASH': '/',
                    'SHOP ON AMAZON': 'https://somelink/',
                    'SHOP AT BIG BAZAAR': '/bigbazaar',
                    'SCHOOL FEES ': 'https://somelink/'
                  },
                  classes
                )}
              </List>
            </Collapse>
            {getNavigationLinks(
              {
                BLOGS: 'https://somelink/',
                NEWS: 'https://somelink',
                FAQS: '/faqs',
                'START YOUR APPLICATION': 'https://somelink/'
              },
              classes
            )}
          </List>
          <Divider />
          <div className={classes['social-container']}>
            <img className={classes['social-container__icon']} src={fbIcon} alt="facebook" />
            <img className={classes['social-container__icon']} src={twitterIcon} alt="twitter" />
            <img className={classes['social-container__icon']} src={linkedinIcon} alt="linkedin" />
          </div>
          <div className={classes['social-container']}>
            <img className={classes['social-container__psicon']} src={androidIcon} alt="android" />
            <img className={classes['social-container__psicon']} src={iosIcon} alt="ios" />
          </div>
        </div>
      );
    }
}
NavigationList.propTypes = {
  classes: PropTypes.object.isRequired
};
export default withStyles(styles)(NavigationList);
\$\endgroup\$
1
\$\begingroup\$

Congratulations on getting a react app to work. That is no small feat. Also nice use of static state and using a function to change state.

I recommend you read thinking in react.

Controls are the basic building blocks of a react application. I recommend creating many small controls and composing them into a larger control. You have one monolithic control that is difficult to change and reason about.

Here is your code with some sub-components:

const HeaderTypeographyRoot = ({ children, classes }) => (
  <Typography
    className={classes["list-container__headertxt"]}
    variant="subtitle2"
    gutterBottom
  >
    {children}
  </Typography>
);
const HeaderTypeography = withStyles(styles)(HeaderTypeographyRoot);

const mapLinks = (navigationObj, className) => linkName => (
  <Link key={linkName} href={navigationObj[linkName]}>
    <ListItem button className={className}>
      <HeaderTypeography>{linkName}</HeaderTypeography>
    </ListItem>
  </Link>
);
const Links = ({ navigationObj, className }) => {
  const links = Object.keys(navigationObj).map(
    mapLinks(navigationObj, className)
  );
  return <React.Fragment>{links}</React.Fragment>;
};
// put in seperate file
const SubheaderRoot = ({ classes }) => (
  <ListSubheader
    className={classes["list-container__subheader"]}
    component="div"
  >
    <div className={classes["list-container__arrow-container"]}>
      <IconButton
        className={classes["list-container__arrowicon"]}
        aria-label="arrow"
      />
    </div>
    <Button
      className={classes["list-container__button"]}
      variant="contained"
      size="large"
      style={{
        color: "#2ec3d2"
      }}
    >
      Download
    </Button>
  </ListSubheader>
);
const Subheader = withStyles(styles)(SubheaderRoot);

class ToggleColapse extends React.Component {
  state = {
    open: false
  };

  handleClick = () => {
    this.setState(state => ({ open: !state.open }));
  };

  render() {
    const { open } = this.state;
    const { children, name } = this.props;
    const icon = open ? "-" : "+";
    return (
      <React.Fragment>
        <ListItem button onClick={this.handleClick}>
          <HeaderTypeography>{name}</HeaderTypeography>
          {icon}
        </ListItem>
        <Collapse in={open} timeout="auto" unmountOnExit>
          <List component="div" disablePadding>
            {children}
          </List>
        </Collapse>
      </React.Fragment>
    );
  }
}

class NavigationList extends React.Component {
  render() {
    const { classes } = this.props;
    return (
      <div className={classes["list-container"]}>
        <List component="nav" subheader={<Subheader />}>
          <Links
            navigationObj={{
              HOME: "/",
              "EXISTING CUSTOMER LOGIN": "https://somelink/",
              "ABOUT Us": "/aboutus",
              CAREERS: "/careers"
            }}
          />
          <ToggleColapse name="Products">
            <Links
              className={classes["list-container__nested"]}
              navigationObj={{
                "INSTANT CASH": "/",
                "SHOP ON AMAZON": "https://somelink/",
                "SHOP AT BIG BAZAAR": "/bigbazaar",
                "SCHOOL FEES ": "https://somelink/"
              }}
            />
          </ToggleColapse>
          <Links
            navigationObj={{
              BLOGS: "https://somelink/",
              NEWS: "https://somelink",
              FAQS: "/faqs",
              "START YOUR APPLICATION": "https://somelink/"
            }}
          />
        </List>
      </div>
    );
  }
}
NavigationList.propTypes = {
  classes: PropTypes.object.isRequired
};
export default withStyles(styles)(NavigationList);

Edit navigation-drawer-code-review

Notice a few things:

I make use of the withStyles higher order function so you don't have to pass so many className parameters around.

getNestedLinks and getNaviationLinks are almost the same thing I made them into one reusable control (Links). Nested links are the same but are indented. This is done by passing in a className property.

Step 4 in thinking in react is: Identify Where Your State Should Live I think it would be better for the is open state to live in the new 'ToggleColapse' control. That way if you wanted a second collapse / nested menu item, you can simply reuse this control.

Next you need to ask, how likely are the links to change, if they are static, favor using static content and don't loop over (getNavigationLinks).

If they are likely to change, separate the values (navigationObj) out of your control. This will make your control easier to understand and if you need to change them, you won't have to hunt through your code.

If you get stuck on this, please ask.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.