trjhtr

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>
 )

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;