Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assignment by ganesh khaire #6

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ganesh-91
Copy link

No description provided.

import { Jumbotron } from "react-bootstrap";
import { Link } from 'react-router-dom';

class Home extends Component {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Components that do not have state can be converted to stateless functional components. Detailed explanation here

class Page404 extends Component {
render() {
return (
<div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can directly return the <Jumbotron> container as done in Home component.

componentDidMount(){
this.getUser();
}
getUser(){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server related code can be refactored into its own separate 'api' module.

render() {
let table =
this.state.userList.map((user ,i)=>{
return ( <TableDataRow data={user} key={i}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component should be written in one single line or into multiple lines as follows for better readability.

<TableDataRow
  data={user}
  key={i}
  delete={this.deleteEntry.bind(this)}
/>

delete={this.deleteEntry.bind(this)}/> );
});
return (
<Jumbotron >

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add proper indentation.

this.setState({ userList: newList });
}
getUseList(){
fetch('https://jsonplaceholder.typicode.com/users')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate the server code into its own 'api' module.

Copy link
Contributor

@javascripters2015 javascripters2015 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use comments wherever required to understand the implementation intent
Avoid long function names
Avoid code duplication , try to make common functions
Code duplication found on
src/components/SingleUser/index.js - line number 31 & 30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants