1
\$\begingroup\$

I'm very new to React.js and have to start converting an entire website at my work. It's fun, but I'm hoping to get some feedback about how I tackled building this navigation component as I don't fully understand best practices when it comes to structuring components as well as proper state and props management.

I have uploaded the full working example to me repo here if you want to clone and run locally: https://github.com/tayloraleach/recursive-react-material-ui-menu

Here are the two components I built that compose the navigation:

The main navigation component that holds all the children

MobileNavigation.jsx

import React from 'react';
import PropTypes from 'prop-types';
import { withStyles } from '@material-ui/core/styles';
import MobileNavigationMenuItem from './MobileNavigationMenuItem';
import classnames from 'classnames';
import List from '@material-ui/core/List';

class MobileNavigation extends React.Component {
    state = {
        currentOpenChildId: null
    };

    handleCurrentlyOpen = (id) => {
        this.setState({
            currentOpenChildId: id
        });
    };

    render() {
        const { classes } = this.props;

        // Loop through the navigation array and create a new component for each,
        // passing the current menuItem and its children as props

        const nodes = this.props.data.navigation.map((item) => {
            return (
                <MobileNavigationMenuItem
                    key={item.id}
                    node={item}
                    passToParent={this.handleCurrentlyOpen}
                    currentlyOpen={this.state.currentOpenChildId}>
                    {item.children}
                </MobileNavigationMenuItem>
            );
        });

        return (
            <List disablePadding className={classnames([this.props.styles, classes.root])}>
                {nodes}
            </List>
        );
    }
}

MobileNavigation.propTypes = {
    classes: PropTypes.object.isRequired,
    styles: PropTypes.string,
    data: PropTypes.object.isRequired
};

const styles = (theme) => ({
    root: {
        width: '100%',
        padding: 0,
        boxShadow: 'inset 0 1px 0 0 rgba(255, 255, 255, 0.15)',
        background: "#222"
    },
    link: {
        color: '#fff',
        textDecoration: 'none'
    }
});

export default withStyles(styles)(MobileNavigation);

And each item of the navigation that gets called recursively

MobileNavigationMenuItem.jsx

import React from 'react';
import { ListItem, Collapse, List } from '@material-ui/core';
import ArrowDropDown from '@material-ui/icons/ArrowDropDown';
import ArrowDropUp from '@material-ui/icons/ArrowDropUp';
import { withStyles } from '@material-ui/core/styles';
import classnames from 'classnames';
import PropTypes from 'prop-types';

class MobileNavigationMenuItem extends React.Component {
    state = {
        open: false,
        id: this.props.node.id,
        currentOpenChildId: null
    };

    handleClick = () => {
        if (this.props.currentlyOpen == this.props.node.id) {
            this.setState((state) => ({ open: !state.open }));
        } else {
            this.setState({ open: true }, this.props.passToParent(this.props.node.id));
        }
    };

    handleCurrentlyOpen = (id) => {
        this.setState({
            currentOpenChildId: id
        });
    };

    // These got separated due to having an inner div inside each item to be able to set a max width and maintain styles
    getNestedBackgroundColor(depth) {
        const styles = {
            backgroundColor: 'rgba(255, 255, 255, 0.05)'
        };
        if (depth === 1) {
            styles.backgroundColor = 'rgba(255, 255, 255, 0.1)';
        }
        if (depth === 2) {
            styles.backgroundColor = 'rgba(255, 255, 255, 0.15)';
        }
        return styles;
    }

    getNestedPadding(depth) {
        const styles = {
            paddingLeft: 0
        };
        if (depth === 1) {
            styles.paddingLeft = 15;
        }
        if (depth === 2) {
            styles.paddingLeft = 30;
        }
        return styles;
    }

    render() {
        const { classes } = this.props;
        let childnodes = null;

        // The MobileNavigationMenuItem component calls itself if there are children
        // Need to pass classes as a prop or it falls out of scope
        if (this.props.children) {
            childnodes = this.props.children.map((childnode) => {
                return (
                    <MobileNavigationMenuItem
                        key={childnode.id}
                        node={childnode}
                        classes={classes}
                        passToParent={this.handleCurrentlyOpen}
                        currentlyOpen={this.state.currentOpenChildId}>
                        {childnode.children}
                    </MobileNavigationMenuItem>
                );
            });
        }

        // Return a ListItem element
        // Display children if there are any
        return (
            <React.Fragment>
                <ListItem
                    onClick={this.handleClick}
                    className={classes.item}
                    style={this.getNestedBackgroundColor(this.props.node.depth)}>
                    <div className={classes.wrapper}>
                            <a
                                href=""
                                style={this.getNestedPadding(this.props.node.depth)}
                                className={classnames([classes.link, !childnodes.length && classes.goFullWidth])}>
                                {this.props.node.title}
                            </a>
                        {childnodes.length > 0 &&
                            (this.props.currentlyOpen == this.props.node.id && this.state.open ? (
                                <ArrowDropUp />
                            ) : (
                                <ArrowDropDown />
                            ))}
                    </div>
                </ListItem>
                {childnodes.length > 0 && (
                    <Collapse
                        in={this.props.currentlyOpen == this.props.node.id && this.state.open}
                        timeout="auto"
                        unmountOnExit>
                        <List disablePadding>{childnodes}</List>
                    </Collapse>
                )}
            </React.Fragment>
        );
    }
}

MobileNavigationMenuItem.propTypes = {
    classes: PropTypes.object.isRequired,
    node: PropTypes.object.isRequired,
    children: PropTypes.array.isRequired,
    passToParent: PropTypes.func.isRequired,
    currentlyOpen: PropTypes.string
};

const styles = (theme) => ({
    link: {
        color: '#fff',
        textDecoration: 'none'
    },
    goFullWidth: {
        width: '100%'
    },
    item: {
        minHeight: 48,
        color: '#fff',
        backgroundColor: 'rgba(255, 255, 255, 0.05)',
        padding: '12px 15px',
        boxShadow: 'inset 0 -1px 0 0 rgba(255, 255, 255, 0.15)',
        '& svg': {
            marginLeft: 'auto'
        }
    },
    wrapper: {
        width: '100%',
        display: 'flex',
        alignItems: 'center',
        maxWidth: '440px', // any value here
        margin: 'auto',
        [theme.breakpoints.down('sm')]: {
            maxWidth: '100%'
        },
    }
});

export default withStyles(styles)(MobileNavigationMenuItem);

I'll admit there is some code clean up I could do in regards to styling nested elements, but overall it works really well and I'm pretty proud of it.

The questions I have stemmed from how I'm closing and opening the children. Each menu item has an open state and acts as a 'parent' of any direct children. When you click an item, it passes the state up and if it the id matches it opens (closing all others).

Each item calls itself if it has children and repeats recursively.

I would love to get some insight on any improvements I can make or if this is a good or bad solution to the problem.

\$\endgroup\$
1
\$\begingroup\$

TL;DR Reworked code : (I used a snippet so I could hide it)

class MobileNavigation extends React.Component {
    state = {
        currentOpenChildId: null
    };

    handleCurrentlyOpen = currentOpenChildId => {
        this.setState({ currentOpenChildId });
    };

    render() {
        const { classes, data: { navigation }, styles } = this.props;

        return (
            <List disablePadding className={classnames([styles, classes.root])}>
                {navigation.map(item => (
                    <MobileNavigationMenuItem
                        key={item.id}
                        node={item}
                        passToParent={this.handleCurrentlyOpen}
                        currentlyOpen={this.state.currentOpenChildId}>
                        {item.children}
                    </MobileNavigationMenuItem>
                ))}
            </List>
        );
    }
}

class MobileNavigationMenuItem extends React.Component {
    state = {
        open: false,
        id: this.props.node.id,
        currentOpenChildId: null
    };

    handleClick = () => {
        const { currentlyOpen, node, passToParent }
        if (currentlyOpen == node.id) {
            this.setState(state => ({ open: !state.open }));
        } else {
            this.setState({ open: true }, passToParent(node.id));
        }
    };

    handleCurrentlyOpen = currentOpenChildId => {
        this.setState({ currentOpenChildId });
    };

    getNestedBackgroundColor = depth => (
        {
            1: 'rgba(255, 255, 255, 0.1)',
            2: 'rgba(255, 255, 255, 0.15)'
        }[depth] || 'rgba(255, 255, 255, 0.05)'
    )

    getNestedPadding = depth => (
        {
            1: 15,
            2: 30
        }[depth] || 0
    )

    render() {
        const { classes, currentlyOpen, node, children } = this.props;
        const { currentOpenChildId, open } = this.state
        
        return (
            <React.Fragment>
                <ListItem
                    onClick={this.handleClick}
                    className={classes.item}
                    style={this.getNestedBackgroundColor(node.depth)}>
                    <div className={classes.wrapper}>
                        <a
                            href=""
                            style={this.getNestedPadding(node.depth)}
                            className={classnames([classes.link, !childnodes.length && classes.goFullWidth])}>
                            {node.title}
                        </a>
                        {children && currentlyOpen == node.id && open ? 
                            <ArrowDropUp />
                            : 
                            <ArrowDropDown />
                        }
                    </div>
                </ListItem>
                {children && (
                    <Collapse
                        in={currentlyOpen == node.id && open}
                        timeout="auto"
                        unmountOnExit>
                        <List disablePadding>
                            {children.map(childnode => (
                                    <MobileNavigationMenuItem
                                        key={childnode.id}
                                        node={childnode}
                                        classes={classes}
                                        passToParent={this.handleCurrentlyOpen}
                                        currentlyOpen={currentOpenChildId}>
                                        {childnode.children}
                                    </MobileNavigationMenuItem>
                            ))}
                        </List>
                    </Collapse>
                )}
            </React.Fragment>
        );
    }
}


Reducing your getXXX functions

3 of your functions share the same layout :

getNestedBackgroundColor(depth) {
    const styles = {
        backgroundColor: 'rgba(255, 255, 255, 0.05)'
    };
    if (depth === 1) {
        styles.backgroundColor = 'rgba(255, 255, 255, 0.1)';
    }
    if (depth === 2) {
        styles.backgroundColor = 'rgba(255, 255, 255, 0.15)';
    }
    return styles;
}

Using JSON objects, you could map each result to the desired depth number :

{
    1: 'rgba(255, 255, 255, 0.1)',
    2: 'rgba(255, 255, 255, 0.15)'
}

Now, just add brackets to extract the correct output, and return the default one if nothing was found using the || operator :

{
    1: 'rgba(255, 255, 255, 0.1)',
    2: 'rgba(255, 255, 255, 0.15)'
}[depth] || 'rgba(255, 255, 255, 0.05)'

The getNestedPadding function :

getNestedPadding = depth => (
    {
        1: 15,
        2: 30
    }[depth] || 0
)

Short syntax : getNestedPadding = depth => ({ 1: 15, 2: 30 }[depth] || 0)

Deconstructing

I added a lot of state and props deconstruction throughout your code :

const { classes, currentlyOpen, node, children } = this.props;
const { currentOpenChildId, open } = this.state

This allows you to stop repeating this.state.XXX later on and make your code more readable.

Conditional rendering

You are already using the && operator with some parameters but are not using it with the map function, your mapped arrays can also be conditionally rendered in your JSX :

return (
    <List disablePadding className={classnames([styles, classes.root])}>
        {navigation.map(item => ( //Short arrow function syntax
            <MobileNavigationMenuItem
                key={item.id}
                node={item}
                passToParent={this.handleCurrentlyOpen}
                currentlyOpen={this.state.currentOpenChildId}>
                {item.children}
            </MobileNavigationMenuItem>
        ))}
    </List>
);

Also, putting single JSX component in a condition does not require using parenthesis :

{children && currentlyOpen == node.id && open ? 
    <ArrowDropUp />
    : 
    <ArrowDropDown />
}

And the variable children can be used instead of childnodes.length > 0 now that your children are conditionally rendered :

<List disablePadding>
    {children.map(childnode => (
            <MobileNavigationMenuItem
                key={childnode.id}
                node={childnode}
                classes={classes}
                passToParent={this.handleCurrentlyOpen}
                currentlyOpen={currentOpenChildId}>
                {childnode.children}
            </MobileNavigationMenuItem>
    ))}
</List>
\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply. Was more interested in feedback surrounding the higher level concept of passing the props and state around. The style functions can be written in numerous ways so wasn't really focusing on them. Wasn't looking to make the code more concise really either but the destructuring, syntax and conditional rendering are things I've noted. \$\endgroup\$ – Taylor A. Leach Feb 3 at 19:38

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.