3
\$\begingroup\$

I've implemented the logic of my app, but I'm not sure whether it appropriate way or not.

Task: create a react app using redux, react-router v4 which has a page with a form for adding new post and list of postspage with form and list of posts

and a page with a detail view of one of the postsdetail view of post

I've added a unique id for each post with help of uuid and pass id of each post to the corresponding link(react-router-dom link) in the post list

      <Link to={props.id} className='post__header'>{props.title}</Link>

When link is clicked it passes id to the DetailView component. DetailView component takes id and seek the post with the same id in the whole state then render the post

import React from 'react';
import { connect } from 'react-redux';

let DetailView = (props) => {
  console.log(props);
  var currentPost = props.posts.find(post => post.id === props.match.params.id);

  return (
    <div>
      <h1>{currentPost.topic}</h1>
      <p>{currentPost.text}</p>
    </div>
  )
}

const mapStateToProps = (state) => {
  return state;
};

DetailView = connect(mapStateToProps)(DetailView);

export default DetailView;

Github link: https://github.com/LKTN/Branch/tree/dev

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

Misc

I'll start with these because to be able to use them in the next sections.

I personally prefer using const, but I won't get into the details, there has been enough debate in the media about it. In this case, I think that declaring the component with let is a bit misleading, because usually, you do this when expanding the API exported with the component, namely:

let List = props => { ... };

List.Item = props => { ... };

export default List;

Which then gets used like this:

const Showcase = () => (
    <List>
        <List.Item>First</List.Item>
        <List.Item>Second</List.Item>
        <List.Item>Third</List.Item>
    </List>
);

So, I would change the declaration of the component to:

const DetailView = (props) => { ... }

export default connect(mapStateToProps)(DetailView);

Props

Destructuring props makes the code a bit more readable because from a glance you can see what the component expects:

let DetailView = ({ posts, match }) => {
  var currentPost = posts.find(post => post.id === match.params.id);
}

var

const and let were introduced two fix a couple of gotchas that javascript had, mostly related to var. You shouldn't use var except in very special cases where you need its behavior (Take this with some salt, because I think that this behavior is confusing and using the alternatives improves the code).

You can read more about it running a simple query;

const currentPost = posts.find(post => post.id === match.params.id);

Redux

State to Props

I see you are mapping your whole redux state to your props. This is a bad practice because that will make your component re-render on every state change. Instead, as recommended in the official docs, you should return only the slice of state that your component uses.

const mapStateToProps = (state) => ({
      props: state.props
});

Hooks

The official docs for react-redux recommend using hooks instead of the connect API. Changing your included code to use hooks would leave use with the following:

import React from 'react';
import { useSelector } from 'react-redux';

const DetailView = ({ match }) => {
  const posts = useSelector(state => state.posts);
  const post = posts.find(post => post.id === match.params.id);

  return (
    <div>
      <h1>{post.topic}</h1>
      <p>{post.text}</p>
    </div>
  )
}

export default DetailView;

Router

I see you are expecting a match prop in your component. You can avoid that prop if you take advantage of the useParams hook that react-router-dom provides:

import React from 'react';
import { useSelector } from 'react-redux';
import { useParams } from 'react-router-dom';

const DetailView = () => {
  const posts = useSelector(state => state.posts);
  const { id } = useParams();
  const post = posts.find(post => post.id === id);

  return (
    <div>
      <h1>{post.topic}</h1>
      <p>{post.text}</p>
    </div>
  )
}

export default DetailView;
```
\$\endgroup\$
0
\$\begingroup\$

Looks good after a quick skim. Just one point. I think this project could benefit from following the Container Component pattern so that the markup components don't directly depend on redux and are reusable.

Read more at https://medium.com/@learnreact/container-components-c0e67432e005

\$\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.