Assignment by saurabh targe#11
Assignment by saurabh targe#11targesaurabh wants to merge 3 commits intojavascripters-community:masterfrom
Conversation
javascripters2015
left a comment
javascripters2015
left a comment
There was a problem hiding this comment.
Please update the code according to the code review instructions.
|
|
||
| `Email : pune.javascripters@gmail.com` | ||
| # online-bidding-react-app | ||
| Online bidding app for paintings built using ReactJS, Webpack dev server and ES2015. |
There was a problem hiding this comment.
This files required details how to use app and setup instruction, so any new member using this project should understand the intent of this code implementation according to the best code practices.
There was a problem hiding this comment.
Okay. I will update the README file with the setup instructions.
| ./client/components/App.jsx | ||
| */ | ||
| import React from 'react'; | ||
|
|
There was a problem hiding this comment.
Comment required here to understand the intent of the code implementation, it holds true for all next implementation too.
There was a problem hiding this comment.
Yes, I will add the appropriate comments in the code.
| <div className="col-md-3 col-sm-6"> | ||
| <div className="product-item"> | ||
| <div className="product-thumb"> | ||
| <img src={this.props.data.image} alt="" style={imageStyle}/> |
There was a problem hiding this comment.
Alt attribute of <img /> empty. Good for Accessibility.
|
|
||
| return ( | ||
| <div> | ||
| <span>Rs. {this.props.highestBid}</span> |
There was a problem hiding this comment.
You can eliminate <div> wrapping and just return <span> element. [Semantic HTML]
| @@ -0,0 +1,134 @@ | |||
| /* | |||
| ./client/components/Timer.jsx | |||
There was a problem hiding this comment.
If possible you can separate out Timer and Clock components into separate files.
|
|
||
| <link href="http://fonts.googleapis.com/css?family=Open+Sans:400,300,600,700,800" rel="stylesheet"> | ||
|
|
||
| <link rel="stylesheet" href="client/css/bootstrap.css"> |
There was a problem hiding this comment.
It's better to import CSS files in js, so that you can perform various transformations using web pack at a later point, or use a cdn for better performance.
| */ | ||
| import React from 'react'; | ||
|
|
||
| export default class App extends React.Component { |
There was a problem hiding this comment.
Can import component along with React and change class defination. eg:
import React, { Component } from 'react';
export default class App extends Component{}
| export default class App extends React.Component { | ||
| render() { | ||
| return ( | ||
| <div style={{textAlign: 'center'}}> |
There was a problem hiding this comment.
If using bootstrap, then can use bootstrap class to make text centered align.
|
|
||
| calculateRemainingTime(){ | ||
|
|
||
| let timeRemaining = this.state.bidEndTime - new Date().getTime(); |
|
|
||
| render() { | ||
|
|
||
| const bidNowButtonStyle = { |
There was a problem hiding this comment.
please declare css classes in an external stylesheet.
|
|
||
| </head> | ||
| <body> | ||
| <header class="site-header"> |
There was a problem hiding this comment.
Header & Menu part can be created in a component and use it in main js file, instead of creating in index.html, it will help to keep index.html clean and short.
4 participants
Please review the branch assignment_by_saurabh_targe.