I'm looking for a review of a recent project I finished for school. It is an app that functions like Twitter. It includes an Express server and Routes. Instead of using databases, it uses JSON files. Users can create, edit, and delete 'chirps' (tweets) with fetch requests. Specific instructions for the project are included on the README.
Please leave any feedback from small mistakes/improvements to big picture stuff like structure and organization. Thank you
My app: https://github.com/ChristyCakes/CovalenceReactBoilerPlate
app.jsx
import React, { Component, Fragment } from 'react';
import { BrowserRouter as Router, Route, Switch } from 'react-router-dom';
import { Home, Chirp, Edit, Delete } from './components';
import './styles.css';
class App extends Component {
render() {
return (
<Router>
<Fragment>
<Switch>
<Route exact path="/" component={Home} />
<Route path="/:id/edit" component={Edit} />
<Route path="/:id/delete" component={Delete} />
<Route path="/:id" component={Chirp} />
</Switch>
</Fragment>
</Router>
)
}
}
export default App;
home.jsx
import React, { Component } from 'react';
import 'isomorphic-fetch';
import Current from './current'
import logo from './logo.png'
import NewChirp from './newChirp'
class Home extends Component {
constructor() {
super();
this.state = { chirps: {} }
}
componentDidMount() {
fetch('http://127.0.0.1:3000/api/chirps')
.then(response => response.json())
.then(data => {
this.setState({ chirps: data })
})
.catch(err => console.log(err))
}
render() {
return (
<div>
<div className="panel">
<img src={logo} alt="logo" height="300px" />
<h1>Chirper</h1>
</div>
<NewChirp />
<Current chirps={this.state.chirps} />
</div>
);
}
}
export { Home };
chirp.jsx
import React, { Component, Fragment } from 'react';
import { BrowserRouter as Router, Link } from 'react-router-dom';
import 'isomorphic-fetch';
class Chirp extends Component {
constructor() {
super();
this.state = {
user: "",
text: "",
}
}
componentDidMount() {
fetch(`http://127.0.0.1:3000/api/chirps/${this.props.match.params.id}`)
.then(response => response.json())
.then(data => {
this.setState({
user: data.user,
text: data.text
})
})
.catch(err => console.log(err))
}
render() {
return (
<div>
<Fragment >
<Link to="/" className="homelink" style={{ textDecoration: "none" }}>Home</Link>
</Fragment>
<div className="current">
<div className="flex-column">
<div className='chirps'>
<p>{this.state.user}: {this.state.text}</p>
<Fragment >
<Link to={{
pathname: `/${this.props.match.params.id}/edit`,
state: {
user: this.state.user,
text: this.state.text
}
}}>
<button onClick={this.editClick}>Edit</button>
</Link>
<Link to={`/${this.props.match.params.id}/delete`}><button className="delete">x</button></Link>
</Fragment>
</div>
</div>
</div>
</div>
)
}
}
export { Chirp };
edit.jsx
import React, { Component, Fragment } from 'react';
import { BrowserRouter as Router, Link } from 'react-router-dom';
import 'isomorphic-fetch';
class Edit extends Component {
constructor() {
super();
this.state = {
user: "",
text: ""
}
this.editChirp = this.editChirp.bind(this);
this.inputHandler = this.inputHandler.bind(this)
}
inputHandler(e) {
this.setState({ [e.target.name]: e.target.value })
}
editChirp(e) {
e.preventDefault();
fetch(`http://127.0.0.1:3000/api/chirps/${this.props.match.params.id}`, {
method: "PUT",
headers: {
"Content-Type": "application/json"
},
body: JSON.stringify({
user: this.state.user,
text: this.state.text
})
})
.then(() => this.props.history.push('/'))
.catch(err => console.log(err))
}
render() {
const user = this.props.location.state.user;
const text = this.props.location.state.text;
return (
<div>
<Fragment>
<Link to="/" className="homelink" style={{ textDecoration: "none" }}>Home</Link>
</Fragment>
<h2>Edit Your Chirp</h2>
<div className="input">
<form action="">
<input
type="text"
placeholder={user}
size="10"
id="user"
name="user"
onChange={this.inputHandler}
value={this.state.user}
/>
<input
type="text"
placeholder={text}
size="60"
id="text"
name="text"
onChange={this.inputHandler}
value={this.state.text}
/>
<button
onClick={this.editChirp}
id="submit">
Update
</button>
</form>
</div>
</div>
)
}
}
export { Edit };
1 Answer 1
Superfluous Fragments
You don't need to wrap anything in a <Fragment>
. Fragments are just used for when you want to return multiple elements without adding anything extra to the DOM.
Exports
In your components, you export an Object (like export { Edit }
) when I think it would be best to just use the default export. So the last line of edit.jsx should become export default Edit
and then change app.jsx to import Edit from './components/Edit'
.
"Smart and Dumb" or "Presentational and Container" components
You mix your business logic and data access in with your view code, which works, but is bad for reusability and general code readability.
Instead, you should have a component that is only concerned with rendering a chirp (i.e. it takes in the chirp as a prop and doesn't fetch it), and another that fetches the data from the API and renders a <Chirp>
with that.
It might look something like this:
// presentational/Chirp.jsx
export default function Chirp(props) {
return (
<div>
<Link to="/" className="homelink" style={{ textDecoration: "none" }}>Home</Link>
<div className="current">
<div className="flex-column">
<div className="chirps">
<p>{props.user}: {props.text}</p>
<Link to={{
pathname: `/${this.props.match.params.id}/edit`,
state: {
user: this.state.user,
text: this.state.text
}
}}>Edit</Link>
<Link to={`/${this.props.match.params.id}/delete`}>x</Link>
</div>
</div>
</div>
</div>
)
}
// container/ChirpContainer.jsx
import Chirp from '../presentational/Chirp'
class ChirpContainer extends Component {
constructor () {
super()
this.state = {
user: '',
text: ''
}
}
componentDidMount () {
fetch(`http://127.0.0.1:3000/api/chirps/${this.props.match.params.id}`)
.then(response => response.json())
.then(data => {
this.setState({
user: data.user,
text: data.text
})
})
.catch(err => console.log(err))
}
render () {
return <Chirp user={this.state.user} text={this.state.text} />
}
}
export default ChirpContainer
Showing an error message
When the call to get data from your API fails, you just print the message out to the console. This is fine for testing and development, but maybe you want to look into showing a message to the user instead, as most of them don't bother to look through the console.
Explore related questions
See similar questions with these tags.