Skip to content
This repository has been archived by the owner. It is now read-only.

Migrate forging component to React - Closes #351 #495

Merged
merged 26 commits into from Jul 24, 2017

Conversation

Projects
4 participants
@slaweet
Copy link
Contributor

commented Jul 18, 2017

Closes #351

@slaweet slaweet self-assigned this Jul 18, 2017

@slaweet slaweet added this to Pending Review in Version 1.1.0 Jul 21, 2017

slaweet added some commits Jul 13, 2017

@slaweet slaweet force-pushed the 351-forging-tab branch from ee4cf8c to 2e64149 Jul 21, 2017

"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",

This comment has been minimized.

Copy link
@yasharAyari

yasharAyari Jul 21, 2017

Contributor

Please remove this dependency and use our custom timestamp component

state.forgedBlocks[state.forgedBlocks.length - 1].timestamp :
0;
return Object.assign({}, state, {
forgedBlocks: [].concat(

This comment has been minimized.

Copy link
@yasharAyari

yasharAyari Jul 21, 2017

Contributor

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

slaweet added some commits Jul 21, 2017

@slaweet slaweet added ready and removed 🏗 in progress labels Jul 21, 2017

@slaweet slaweet requested review from reyraa and yasharAyari Jul 21, 2017

@reyraa
Copy link
Contributor

left a comment

Very neat and good job Vit, Thanks

{
key: 'rate',
label: 'Rank',
textForPercentage: pct => (101 - pct),

This comment has been minimized.

Copy link
@reyraa

reyraa Jul 24, 2017

Contributor

Webpack will minify later in production. please use human readable variable names.

This comment has been minimized.

Copy link
@reyraa

reyraa Jul 24, 2017

Contributor

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}>

This comment has been minimized.

Copy link
@reyraa

reyraa Jul 24, 2017

Contributor

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 = [

This comment has been minimized.

Copy link
@reyraa

reyraa Jul 24, 2017

Contributor

ariable names better not defined based on data structure. please use progressCircleCardList


chai.use(sinonChai);

describe('<DelegateStats />', () => {

This comment has been minimized.

Copy link
@reyraa

reyraa Jul 24, 2017

Contributor

Please remove html annotation


chai.use(sinonChai);

describe('<ForgedBlocks />', () => {

This comment has been minimized.

Copy link
@reyraa

reyraa Jul 24, 2017

Contributor

Please remove html annotation

chai.use(sinonChai);


describe('<ForgingStats />', () => {

This comment has been minimized.

Copy link
@reyraa

reyraa Jul 24, 2017

Contributor

Same here

chai.use(sinonChai);


describe('<ForgingTitle />', () => {

This comment has been minimized.

Copy link
@reyraa

reyraa Jul 24, 2017

Contributor

same here

});

const mapDispatchToProps = dispatch => ({
loadForgedBlocks: (activePeer, limit, offset, generatorPublicKey) => {

This comment has been minimized.

Copy link
@reyraa

reyraa Jul 24, 2017

Contributor

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.

});
},
loadStats: (activePeer, key, startMoment, generatorPublicKey) => {
getForgedStats(activePeer, startMoment, generatorPublicKey).then((data) => {

This comment has been minimized.

Copy link
@reyraa

reyraa Jul 24, 2017

Contributor

same here

slaweet added some commits Jul 24, 2017

@slaweet slaweet force-pushed the 351-forging-tab branch from aebe4d0 to 000e534 Jul 24, 2017

@slaweet slaweet added 🏗 in progress and removed ready labels Jul 24, 2017

slaweet added some commits Jul 24, 2017

@slaweet slaweet added ready and removed 🏗 in progress labels Jul 24, 2017

@reyraa

reyraa approved these changes Jul 24, 2017

Copy link
Contributor

left a comment

Thank you Vit!

@slaweet slaweet merged commit 42de0c7 into development Jul 24, 2017

2 checks passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
coverage/coveralls Coverage remained the same at 80.285%
Details

@slaweet slaweet deleted the 351-forging-tab branch Jul 24, 2017

@slaweet slaweet moved this from Pending Review to Done in Version 1.1.0 Jul 24, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.