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

200_success
145k22 gold badges190 silver badges478 bronze badges
asked Dec 25, 2017 at 8:43
\$\endgroup\$

2 Answers 2

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;
```
answered Jul 5, 2021 at 0:32
\$\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

answered Oct 13, 2019 at 17:11
\$\endgroup\$
1
  • \$\begingroup\$ This pattern of splitting components is no more the best way. Dan Abramov introduced this in this article Presentational and Container Components but he don't recommend it anymore. \$\endgroup\$ Commented Jun 5, 2022 at 13:28

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.