Create priced button component - Closes #583 #585

reyraa · 2017-08-08T15:37:43Z

Closes #583

…ants

slaweet · 2017-08-08T15:42:24Z

src/constants/fees.js

+export default {
+  setSecondPassphrase: 5e8,
+  send: 1e7,
+  registerDelegate: 25e8,


vote: 1e8. is missing

slaweet · 2017-08-08T15:45:24Z

src/components/pricedButton/index.js

+      }
+      <Button label={props.label}
+        primary={true} raised={true}
+        className='next-button'


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

willclarktech

A few minor comments.

willclarktech · 2017-08-09T08:36:24Z

src/constants/fees.js

@@ -0,0 +1,6 @@
+export default {
+  setSecondPassphrase: 5e8,
+  send: 1e7,


Prefer 0.1e8 for uniformity.

willclarktech · 2017-08-09T08:36:47Z

src/components/pricedButton/pricedButton.css

+  transition: all 0.3s cubic-bezier(0.55, 0, 0.55, 0.2);
+}
+.error {
+  color: #dd2c00 !important;


Is there a way to avoid the !important?

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

willclarktech · 2017-08-09T08:38:04Z

src/components/pricedButton/index.test.js

+import PricedButton from './index';
+
+chai.use(sinonChai);
+chai.use(chaiEnzyme());


Do you guys do this in every file?

willclarktech · 2017-08-09T08:39:52Z

src/components/pricedButton/index.test.js

+    fee: 5e8,
+    onClick: sinon.spy(),
+  };
+  const insufficientBalance = 0;


Prefer the largest possible insufficient balance.

willclarktech · 2017-08-09T08:41:28Z

src/components/pricedButton/index.test.js

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


Prefer .calledWithExactly.

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

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

replaced with calledWithExactly.

willclarktech · 2017-08-09T08:44:20Z

src/components/pricedButton/index.js

+import { fromRawLsk } from '../../utils/lsk';
+import styles from './pricedButton.css';
+
+const PricedButton = (props) => {


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

willclarktech · 2017-08-09T08:45:30Z

src/components/pricedButton/index.js

+    <div className='primary-button'>
+      {
+        props.fee &&
+          <span className={`${styles.fee} ${hasFunds ? '' : styles.error}`}>


Prefer parentheses around span.

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

willclarktech · 2017-08-09T08:46:59Z

src/components/pricedButton/index.js

+          </span>
+      }
+      <Button label={props.label}
+        primary={true} raised={true}


Prefer separate lines for separate props.

willclarktech · 2017-08-09T08:47:37Z

src/components/pricedButton/index.js

+        primary={true} raised={true}
+        className={`next-button ${props.customClassName}`}
+        disabled={props.disabled || (props.fee && !hasFunds)}
+        onClick={props.onClick.bind(this)}/>


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

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

What are you expecting the value of this to be?

karmacoma · 2017-08-09T08:43:39Z

src/components/pricedButton/index.test.js

+    expect(wrapper.find(Button)).to.have.length(1);
+  });
+
+  describe('Sufficient credit', () => {


Sufficient funds

karmacoma · 2017-08-09T08:43:52Z

src/components/pricedButton/index.test.js

+    });
+  });
+
+  describe('Insufficient credit', () => {


Insufficient funds

karmacoma · 2017-08-09T08:44:45Z

src/components/pricedButton/pricedButton.css

+  text-align: right;
+  margin: 0 16px;
+  transition: all 0.3s cubic-bezier(0.55, 0, 0.55, 0.2);
+}


Space between selectors.

karmacoma · 2017-08-09T08:46:35Z

src/components/pricedButton/index.js

+          <span className={`${styles.fee} ${hasFunds ? '' : styles.error}`}>
+            {
+              hasFunds ? `Fee: ${fromRawLsk(props.fee)} LSK` :
+              `Not enough credit to pay ${fromRawLsk(props.fee)} LSK fee`


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

karmacoma · 2017-08-09T08:51:34Z

src/components/pricedButton/index.js

+        primary={true} raised={true}
+        className={`next-button ${props.customClassName}`}
+        disabled={props.disabled || (props.fee && !hasFunds)}
+        onClick={props.onClick.bind(this)}/>


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

willclarktech

Nice

- Create priced button. - Add unit test coverage. - Add fees to const…

9140892

…ants

reyraa self-assigned this Aug 8, 2017

reyraa added the enhancement label Aug 8, 2017

slaweet reviewed Aug 8, 2017

View reviewed changes

Minor fixings

7678e16

reyraa added the 👁️ pending review label Aug 9, 2017

reyraa requested review from karmacoma and alepop August 9, 2017 08:44

willclarktech suggested changes Aug 9, 2017

View reviewed changes

karmacoma suggested changes Aug 9, 2017

View reviewed changes

Code cleanup and minor fixings

90bfb50

karmacoma previously approved these changes Aug 9, 2017

View reviewed changes

Use calledWithExactly instead of calledWith

2fd1c3d

reyraa dismissed karmacoma’s stale review via 2fd1c3d August 9, 2017 09:19

alepop approved these changes Aug 9, 2017

View reviewed changes

Fix the test title and remove binding

49f2878

willclarktech approved these changes Aug 9, 2017

View reviewed changes

reyraa merged commit b670d62 into development Aug 9, 2017

reyraa deleted the 583-priced-button branch August 9, 2017 09:58

karmacoma added ✅ ready and removed 👁️ pending review bug chore enhancement update test refactoring labels Aug 9, 2017

Create priced button component - Closes #583 #585

Create priced button component - Closes #583 #585

Conversation

reyraa commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willclarktech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alepop Aug 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willclarktech left a comment

Choose a reason for hiding this comment

alepop Aug 9, 2017 •

edited

Loading