2
\$\begingroup\$

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 };
Sᴀᴍ Onᴇᴌᴀ
29.5k16 gold badges45 silver badges201 bronze badges
asked Oct 3, 2018 at 19:37
\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

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.

answered Oct 3, 2018 at 21:50
\$\endgroup\$

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.