Problem
This was written for a coding challenge for a company I recently starting working for. I’m looking for any suggestions on how to clean up the code, as well as any issues anyone thinks may occur as it is written now. Fairly new to React.js, always looking to improve.
Currently the data is passed through the app component where I render the Nav Container (functional stateless component). The Nav container creates the classNames and passes data + classNames to the NestedNav that I’ve put below.
It started to feel a little sloppy while mapping twice below the render, not sure if that’s a problem?
The data object example shows how I modeled my data. This example has a parent with children that also have children.
- The parentClass is for the outer parent.
- The nestedParentClass is for the children of the outer parent.
- The childClass is reserved for any children of the nestedParent.
export const data = [
{
id: 1,
icon: <FontAwesomeIcon className='i' icon={faHeartbeat} />,
title: 'SAFETY',
children: [
{title: 'Reporting', children: [
{id: 11, title: 'I-21 Injury Reporting', url: '/safety/report'},
{id: 12, title: 'ASAP Reporting', url: '/safety/asap-report'},
{id: 13, title: 'General ASAP Information', url: '/safety/general'},
{id: 14, title: 'Flight Attendant Incident Report', url: '/safety/flight-attendant-incident'},
]},
{title: 'Agriculture and Customs', children: [
{id: 15, title: 'Product Safety Data Search', url: '/safety/data-search'}]},
{id: 16, title: 'Known Crewmember', url: '/safety/known-cremember'},
{id: 17, title: 'Product Safety Data Search', url: '/safety/data-search'},
],
},
]
class NestedNav extends Component {
constructor(props) {
super(props)
this.state = {
selected: '',
nestSelect: '',
children: [],
active: '',
}
}
handleClick = (title) => {
this.setState({
selected: '',
nestSelect: '',
children: [],
active: '',
})
}
render() {
const {data, parentClass, nestedParentClass} = this.props
const renderedChildElements = this.state.children
const active = this.state.active === 'true' ? 'true' : ''
const mappedChildren = (child, title) => {
let childElement
if(child) {
childElement = child.map((child, i) => <li
key={i}
id={child.id}
className={nestedParentClass + (this.state.nestSelect === child.title ? 'nest-selected' : '')}
url={child.url}><Link to={'/'}>{child.title}</Link>
{child.children ?
<FontAwesomeIcon
onClick={() => mappedChildren(child.children, this.state.select)}
className='i button'
icon={faArrowCircleDown} /> : null}
</li>)
this.setState({
selected: title,
children: childElement,
active: 'true',
})
}
return ''
}
const navListItems = data.map((collection, i) => <li
key={i}
id={collection.id}
className={parentClass + (this.state.selected === collection.title ? 'selected' : '')}
url={collection.url}><i onClick={this.handleClick}>{collection.icon}</i><Link to={'/'}><span>{collection.title}</span></Link>
{collection.children ?
<FontAwesomeIcon
onClick={() => mappedChildren(collection.children, collection.title)}
className='i button'
icon={faArrowCircleRight} /> : null}
</li>)
return (
<div className={'nested-nav'}>
<div className={'container-two-' + active}>
<h2>{this.state.selected}</h2>
{this.state.children}
</div>
<div className='container-one'>{navListItems}</div>
</div>
)
}
}
export default NestedNav
Solution
it’s clear and readable but i would like to point out some things :
Avoid using functions inside Render()
A function in the render method will be created each render which is a
slight performance hit. t’s also messy if you put them in the render, which is a much > bigger reason
Use the child.id
instead of key
as index
when you map
through an array in render()
because Index as a key is an anti-pattern.
Keep your code as DRY as possible, the initialState can be an object outside the class instead of writing it twice or more, especially if it’s a big object.
Optional : use Destructuring assignment to indicate which keys you will use in the function.
const initialState = {
selected: '',
nestSelect: '',
children: [],
active: '',
}
class NestedNav extends Component {
constructor(props) {
super(props)
this.state = initialState
}
handleClick = title => {
this.setState({ ...initialState })
}
mappedChildren = (child, selectedTitle) => {
const { nestedParentClass } = this.props
let childElement
if (child) {
childElement = child.map(({ id, title, children, url }) => <li
key={id}
id={id}
className={nestedParentClass + (this.state.nestSelect === title ? 'nest-selected' : '')}
url={url}><Link to={'/'}>{title}</Link>
{children ?
<FontAwesomeIcon
onClick={() => this.mappedChildren(children, this.state.select)}
className='i button'
icon={faArrowCircleDown} /> : null}
</li>)
this.setState({
selected: selectedTitle,
children: childElement,
active: 'true',
})
}
return ''
}
navListItems = data => data.map(({ id, url, children, title, icon }) => {
const { parentClass } = this.props
return (<li
key={id}
id={id}
className={parentClass + (this.state.selected === title ? 'selected' : '')}
url={url}><i onClick={this.handleClick}>{icon}</i><Link to={'/'}><span>{title}</span></Link>
{children ?
<FontAwesomeIcon
onClick={() => this.mappedChildren(children, title)}
className='i button'
icon={faArrowCircleRight} /> : null}
</li>)
});
render() {
const { data } = this.props
const active = this.state.active === 'true' ? 'true' : ''
return (
<div className={'nested-nav'}>
<div className={'container-two-' + active}>
<h2>{this.state.selected}</h2>
{this.state.children}
</div>
<div className='container-one'>{this.navListItems(data)}</div>
</div>
)
}
}
export default NestedNav