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 saurabh targe #11
base: master
Are you sure you want to change the base?
Assignment by saurabh targe #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alt attribute of <img />
empty. Good for Accessibility.
|
||
return ( | ||
<div> | ||
<span>Rs. {this.props.highestBid}</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If using bootstrap, then can use bootstrap class to make text centered align.
|
||
calculateRemainingTime(){ | ||
|
||
let timeRemaining = this.state.bidEndTime - new Date().getTime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary spaces can be removed
|
||
render() { | ||
|
||
const bidNowButtonStyle = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please declare css classes in an external stylesheet.
|
||
</head> | ||
<body> | ||
<header class="site-header"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Please review the branch assignment_by_saurabh_targe.