React Filter library

Question

This is my first small react app. I got a little experience in Vue.js before, but I wan't to learn the basics and best practices of react.js too. So, this is a small library app (containing link tipps with title, desc, topics) that shows all library items with the possibility to filter them by clicking on specific keywords. Pretty easy, but I want to know from you if I coded some bad practices or anti patterns or sth. like that.

main.js

import React from 'react'
import ReactDOM from 'react-dom'
import Library from './components/Library.jsx'
const libraryContainer = document.getElementById('library')
ReactDOM.render(<Library />, libraryContainer)

Library.jsx

import React from 'react'
import Filters from './Filters.jsx'
import LibraryItem from './LibraryItem.jsx'
import SmoothScroll from 'smooth-scroll'

const libraryContainer = document.getElementById('library')
const scroll = new SmoothScroll('a[href*="#"]', { offset: 100 })

export default class Library extends React.Component {
  state = {
    lib: [],
    filteredLib: [],
    topics: [],
    filter: '',
  }

  filterLib = () => {
    if (this.state.filter === '') return this.setState({ filteredLib: this.state.lib })

    this.setState(
      {
        filteredLib: this.state.lib.filter(item => {
          if (item.topics) return item.topics.includes(this.state.filter)
        }),
      },
      () => scroll.animateScroll(libraryContainer),
    )
  }

  handleFilters = topic => {
    const topicsClone = JSON.parse(JSON.stringify(this.state.topics))
    topicsClone.forEach(item => {
      if (item.id === topic.id) {
        item.active = !item.active
        item.active ? null : (topic.name = '')
      } else {
        item.active = false
      }
    })
    this.setState({ topics: topicsClone, filter: topic.name }, () => {
      this.filterLib()
    })
  }

  componentDidMount() {
    let topicsArr = []

    fetch('/api')
      .then(res => res.json())
      .then(data => {
        this.setState({ lib: data.lib, filteredLib: data.lib })
        data.topics.map((val, key) => {
          topicsArr.push({ name: val, id: key, active: false })
        })
        this.setState({ topics: topicsArr })
      })
      .catch(err => console.log(err))
  }

  render() {
    if (this.state.filteredLib.length > 0) {
      return (
        <div className="library-container">
          <Filters topics={this.state.topics} handler={this.handleFilters} />

          <div className="column column--content">
            <ul className="cards">
              {this.state.filteredLib.map(item => {
                return <LibraryItem key={item._id} {...item} />
              })}
            </ul>
          </div>
        </div>
      )
    } else {
      return <h4>Wir konnten leider nichts finden :(</h4>
    }
  }
}

Filters.jsx

import React from 'react'

export default function Filters(props) {
  const handleClick = topic => {
    props.handler(topic)
  }

  return (
    <div className="column column--filters">
      <h3 className="title">Filter</h3>
      <ul className="filters">
        {props.topics.map(topic => {
          return (
            <li key={topic.id} className={topic.active ? 'filters__item active' : 'filters__item'} onClick={() => handleClick(topic)}>
              {topic.name}
            </li>
          )
        })}
      </ul>
    </div>
  )
}

LibraryItem.jsx

import React from 'react'

export default function LibraryItem(props) {
  return (
    <li className="cards__item card">
      <a className="card__link" href={props.slug} />
      <div className="card__header">
        <p className="type">{props.type}</p>
        <h3 className="card__title">{props.title}</h3>
      </div>
      <div className="card__content">
        <p className="content">{props.teaser}</p>
      </div>
    </li>
  )
}

Jamal · Accepted Answer · 2018-01-12 03:11:06Z

Here is a review of the code. While some my comments / suggestions are related to JavaScript and not to react specifically, these do reflect common practices in React code.

I put a live version with the changes on codesandbox. I created a small API to have some data for the fetch request. I also added a css file to test the active behavior for the selected filter topics.

Library.jsx

componentDidMount():

It is a common practice in react to use the map function to create a new array. Also, you can change the code so that setState is only called once (for conciseness):

componentDidMount() {
  fetch("https://demo5925186.mockable.io")
    .then(res => res.json())
    .then(data => {
      const topics = data.topics.map((name, index) => ({
        name,
        id: index,
        active: false
      }));

      this.setState({
        lib: data.lib,
        filteredLib: data.lib,
        topics
      });
    })
    .catch(err => console.log(err));
}

render()

It can make your JSX easier to read if you extract properties from the state on the first line using object destructuring assignment:

const { filteredLib, topics } = this.state;

It seems like you would always want to show the Filters component (the topic selection), even if filteredLib.length is equal to 0.

render() {
    const { filteredLib, topics } = this.state;
    return (
      <div className="library-container">
        <Filters topics={topics} handleClick={this.handleFilters} />
        {filteredLib.length === 0 ? (
              <h4>Wir konnten leider nichts finden :(</h4>
        ) : (
          <div className="column column--content">
            <ul className="cards">
              {filteredLib.map(item => (
                <LibraryItem key={item._id} {...item} />
              ))}
            </ul>
          </div>
        )}
      </div>
    );
}

handleFilters

I changed state.filter to state.selectedTopic and the input argument name from topic to newTopic.

Similarly to componentDidMount, the newTopics array can be created with map (you could convert the ternary to an if-else statement). You can use the spread operator on each object to create a new object.

It seems like state.filter would never be an empty string because handleFilters always sets state.filter to whichever topic was selected. To fix this, you can compared the new selected topic to the existing one. If they are the same, the new selected topic should be set to an empty string.

handleFilters = newTopic => {
    const { topics, selectedTopic } = this.state;

    const newTopics = topics.map(
      topic =>
        topic.id === newTopic.id
          ? { ...topic, active: !topic.active }
          : { ...topic, active: false }
    );

    // If the new selected topic is same as the old one
    // We will set state.selectedTopic to ""
    const newSelectedTopic =
      newTopic.name === selectedTopic ? "" : newTopic.name;

    this.setState(
      {
        topics: newTopics,
        selectedTopic: newSelectedTopic
      },
      () => {
        this.filterLib();
      }
    );
  };

Filters.jsx

It is common to define functional components with const, and the parameters can be destructured in the function definition.

There is no need for the handleClick method. You can change the name of props.handler to props.handleClick, and call it directly.

const Filters = ({ topics, handleClick }) => (
  <div className="column column--filters">
    <h3 className="title">Filter</h3>
    <ul className="filters">
      {topics.map(topic => (
        <li
          key={topic.id}
          className={
            topic.active ? "filters__item active" : "filters__item"
          }
          onClick={() => handleClick(topic)}
        >
          {topic.name}
        </li>
      ))}
    </ul>
  </div>
);

export default Filters;

Wow thank you Lev Izraelit! Your answer is awesome and helps me a lot to get better at react. I'll study every line and improve my code. Can't thank you enough. — FNGR
– FNGR, Commented Jan 13, 2018 at 8:33

Stack Exchange Network

React Filter library

1 Answer 1

You must log in to answer this question.

Hot Network Questions

React Filter library

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions