-
Notifications
You must be signed in to change notification settings - Fork 60
Migrate forging component to React - Closes #351 #495
Conversation
package.json
Outdated
"react-dom": "=15.6.x", | ||
"react-redux": "=5.0.3", | ||
"react-redux-toastr": "=7.0.0", | ||
"react-router-dom": "=4.0.0", | ||
"react-timeago": "=3.3.0", |
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 remove this dependency and use our custom timestamp component
src/store/reducers/forging.js
Outdated
state.forgedBlocks[state.forgedBlocks.length - 1].timestamp : | ||
0; | ||
return Object.assign({}, state, { | ||
forgedBlocks: [].concat( |
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.
Replace [].concat with es6 spread syntax. You can find more about spread syntax here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator
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.
Very neat and good job Vit, Thanks
{ | ||
key: 'rate', | ||
label: 'Rank', | ||
textForPercentage: pct => (101 - pct), |
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.
Webpack will minify later in production. please use human readable variable names.
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.
Also if textForPercentage
and percentageTransform
are doing the exact same job, why 2 functions?
const DelegateStats = props => ( | ||
<div className={`${grid.row} ${grid['between-xs']}`}> | ||
{progressCircleCardObjects.map(cardObj => ( | ||
<div className={grid['col-xs-4']} key={cardObj.key}> |
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.
Variable names better not defined based on data structure. please use cardItem
import grid from '../../../node_modules/flexboxgrid/dist/flexboxgrid.css'; | ||
import style from './forging.css'; | ||
|
||
const progressCircleCardObjects = [ |
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.
ariable names better not defined based on data structure. please use progressCircleCardList
|
||
chai.use(sinonChai); | ||
|
||
describe('<DelegateStats />', () => { |
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 remove html annotation
|
||
chai.use(sinonChai); | ||
|
||
describe('<ForgedBlocks />', () => { |
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 remove html annotation
chai.use(sinonChai); | ||
|
||
|
||
describe('<ForgingStats />', () => { |
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.
Same here
chai.use(sinonChai); | ||
|
||
|
||
describe('<ForgingTitle />', () => { |
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.
same here
src/components/forging/index.js
Outdated
}); | ||
|
||
const mapDispatchToProps = dispatch => ({ | ||
loadForgedBlocks: (activePeer, limit, offset, generatorPublicKey) => { |
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.
To keep our code style consistent, Please follow the approach in login, transaction and account components by moving the Api call to the component consuming the data, and only dispatching the data here.
src/components/forging/index.js
Outdated
}); | ||
}, | ||
loadStats: (activePeer, key, startMoment, generatorPublicKey) => { | ||
getForgedStats(activePeer, startMoment, generatorPublicKey).then((data) => { |
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.
same here
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.
Thank you Vit!
Closes #351