Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Create priced button component - Closes #583 #585

Merged
merged 5 commits into from
Aug 9, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/components/pricedButton/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import React from 'react';
import Button from 'react-toolbox/lib/button';
import { fromRawLsk } from '../../utils/lsk';
import styles from './pricedButton.css';

const PricedButton = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

A pattern I like is to destructure props out here for less verbosity in the component body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const hasFunds = props.balance >= props.fee;
return (
<div className='primary-button'>
{
props.fee &&
<span className={`${styles.fee} ${hasFunds ? '' : styles.error}`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer parentheses around span.

Copy link
Contributor

@alepop alepop Aug 9, 2017

Choose a reason for hiding this comment

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

Maybe it is a better solution to change classnames based on the state https://github.com/JedWatson/classnames

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
hasFunds ? `Fee: ${fromRawLsk(props.fee)} LSK` :
`Not enough credit to pay ${fromRawLsk(props.fee)} LSK fee`
Copy link
Contributor

Choose a reason for hiding this comment

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

Insufficient funds for ${fromRawLsk(props.fee)} LSK fee

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
</span>
}
<Button label={props.label}
primary={true} raised={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer separate lines for separate props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

className='next-button'
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be more flexible, e.g. className={props.className}

disabled={props.disabled || (props.fee && !hasFunds)}
onClick={props.onClick.bind(this)}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is binding doing anything here? Seems like it should be the responsibility of whatever is passing the function down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

binding is the repressibility of presentational component. I believe it's the right place for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What are you expecting the value of this to be?

Copy link
Contributor

Choose a reason for hiding this comment

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

onClick={props.onClick.bind(this)} />

</div>
);
};

export default PricedButton;
56 changes: 56 additions & 0 deletions src/components/pricedButton/index.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import React from 'react';
import chai, { expect } from 'chai';
import sinonChai from 'sinon-chai';
import sinon from 'sinon';
import { shallow } from 'enzyme';
import chaiEnzyme from 'chai-enzyme';
import Button from 'react-toolbox/lib/button';
import PricedButton from './index';

chai.use(sinonChai);
chai.use(chaiEnzyme());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you guys do this in every file?

Copy link
Contributor

Choose a reason for hiding this comment

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


describe.only('DialogElement', () => {
let wrapper;
const props = {
fee: 5e8,
onClick: sinon.spy(),
};
const insufficientBalance = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer the largest possible insufficient balance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const sufficientBalance = 6e8;

it('renders <Button /> component from react-toolbox', () => {
wrapper = shallow(<PricedButton {...props} balance={sufficientBalance} />);
expect(wrapper.find(Button)).to.have.length(1);
});

describe('Sufficient credit', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sufficient funds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

beforeEach(() => {
wrapper = shallow(<PricedButton {...props} balance={sufficientBalance} />);
});

it('renders a span saying "Fee: 5 LSK"', () => {
expect(wrapper.find('span').text()).to.be.equal('Fee: 5 LSK');
});

it('allows to click on Button', () => {
wrapper.find(Button).simulate('click');
expect(props.onClick).to.have.been.calledWith();
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer .calledWithExactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component is not supposed to pass anything to it's click handler. what's the difference?

Copy link
Contributor

Choose a reason for hiding this comment

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

screen shot 2017-08-09 at 11 09 25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component is not passing anything. this is about the difference of calling with different arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replaced with calledWithExactly.

});
});

describe('Insufficient credit', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Insufficient funds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

beforeEach(() => {
wrapper = shallow(<PricedButton {...props} balance={insufficientBalance} />);
});

it('renders a span saying "Not enough credit to pay 5 LSK fee"', () => {
expect(wrapper.find('span').text()).to.be.equal('Not enough credit to pay 5 LSK fee');
});

it('sets the disabled attribute of the button', () => {
const buttonProps = wrapper.find(Button).props();
expect(buttonProps.disabled).to.be.equal(true);
});
});
});
11 changes: 11 additions & 0 deletions src/components/pricedButton/pricedButton.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
.fee {
font-size: 12px;
line-height: 14px;
color: grey;
text-align: right;
margin: 0 16px;
transition: all 0.3s cubic-bezier(0.55, 0, 0.55, 0.2);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Space between selectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.error {
color: #dd2c00 !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to avoid the !important?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need. Preferred is that states override modules. https://smacss.com/

}
5 changes: 5 additions & 0 deletions src/constants/fees.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export default {
setSecondPassphrase: 5e8,
send: 1e7,
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer 0.1e8 for uniformity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

registerDelegate: 25e8,
Copy link
Contributor

Choose a reason for hiding this comment

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

vote: 1e8. is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

};