Add stripe business accounts #980

OtterleyW · 2018-12-17T12:39:48Z

PayoutDetailsForm is now divided to multiple smaller subcomponents:

PayoutDetailsCompany and PayoutDetailsIndividual contain needed fields for individual and company accounts
PayoutDetailsAddress ,PayoutDetailsBankDetails and PayoutDetailsPersonalDetails specific fields related to address, bank details and personal details

New FieldArray component is created for adding multiple additional owners if needed.

Select individual or company account

Add additional owners

Gnito · 2018-12-31T10:24:43Z

src/forms/PayoutDetailsForm/PayoutDetailsPersonalDetails.js

+  disabled: bool,
+  fieldId: string,
+
+  // from injectIntl


Btw. this is a bit confusing comment since there is no injectIntl (higher order component) used in this file.

Gnito · 2018-12-31T10:31:10Z

src/components/FieldArray/FieldArray.css

+.root {
+}
+
+.formRow {


This is not used.

Gnito · 2018-12-31T10:31:51Z

src/components/FieldArray/FieldArray.css

+  margin-bottom: 24px;
+}
+
+.field {


And this is not used either.

Gnito · 2018-12-31T10:37:05Z

src/components/FieldArray/FieldArray.css

+.field {
+  width: 100%;
+}
+.buttonsWrapper button {


This is used in the example only. (We try to keep example/styleguide CSS out of main component CSS by having separate ComponentName.example.css file).

Another thing to note: we don't want to refer to elements anywhere except in marketplaceIndex.css file - add classes instead.

Gnito · 2018-12-31T10:51:08Z

src/components/FieldArray/FieldArray.js

+  const classes = classNames(rootClassName || css.root, className);
+
+  return (
+    <FieldArray id={id} name={name} className={classes}>


Sorry, but I don't think it makes much sense for this to exists as a separate component (FieldArray wrapper for FieldArray). There are no styles attached to this component (and the styles are likely to be different in different use scenarios when import { FieldArray } from 'react-final-form-arrays'; is used.

It is also good to note that every time, when FieldArrays are used inside Final Form, the form needs to pass arrayMutators as props to the FinalForm component itself. So, wrapping the usage of react-final-form-arrays library with separate component like this one might be a bit confusing since it doesn't work out of the box.

Gnito · 2018-12-31T11:04:55Z

src/components/IconClose/IconClose.js

  const classes = classNames(rootClassName || css.root, className);

+  // Use given size as width and height or default to 12px
+  const svgSize = size ? size : 12;


There's a bit of catch when sizing SVG icons with width and height. They render graphics (paths, rects, circles etc.) to their own inner viewbox (or should I say coordinate system).

There's one problem with this concept related to line graphics: if the width of the icon is scaled down, also the line width is scaled down accordingly. So, it is unlikely that the width of the line matches with the width our designer intended it to be. (This is the reason why we have different SVG graphics for different sizes in IconArrowHead.js

Gnito · 2018-12-31T11:32:22Z

src/ducks/user.duck.js

+    payoutDetailValues = payoutDetails['company'];
+  } else if (accountType === 'individual') {
+    payoutDetailValues = payoutDetails['individual'];
+  }


This is OK but can be achieved with ternary too.

Gnito · 2018-12-31T11:40:48Z

src/forms/PayoutDetailsForm/PayoutDetailsForm.js

-          id: 'PayoutDetailsForm.firstNameRequired',
-        })
-      );
+      const { country } = values;


You could extract accountType here too since it is used later.

Gnito · 2018-12-31T11:41:52Z

src/forms/PayoutDetailsForm/PayoutDetailsForm.js

-              />
-            </div>
+          {values.accountType ? (
+            <React.Fragment>


Gnito · 2018-12-31T11:54:39Z

src/forms/PayoutDetailsForm/PayoutDetailsFormCompany.js

+
+  const hasAdditionalOwners = values.company && values.company.additionalOwners;
+  const hasMaxNumberOfAdditionalOwners =
+    hasAdditionalOwners && values.company.additionalOwners.length >= 4;


We don't want to use magic numbers in the code. So, I'd suggest that this number is either

coming from config: maxNumberOfAdditionalOwners (might be unnecessary if the value is just 4 or nothing) or

it is a constant defined at the beginning of this file:

// In EU, there can be a maximum of 4 additional owners const MAX_NUMBER_OF_ADDITIONAL_OWNERS = 4;

Gnito

I like it how much easier this is to read and configure after separating individual and company sections and adding more sub components.

I added quite many change request related to code-conventions we use (and pointed some unused code), but actually, this looks pretty good 👍

Gnito

LGTM, check Janne's opinion before merging

…etailsFormCompany

OtterleyW added the in progress label Dec 17, 2018

OtterleyW force-pushed the add-stripe-business-accounts branch 11 times, most recently from b50df40 to 8b16f86 Compare December 21, 2018 13:35

OtterleyW added review wanted and removed in progress labels Dec 21, 2018

OtterleyW requested a review from Gnito December 21, 2018 14:21

Gnito reviewed Dec 31, 2018

View reviewed changes

src/components/FieldArray/FieldArray.css Outdated

.root {

}

.formRow {

Copy link

Contributor

Gnito Dec 31, 2018

Choose a reason for hiding this comment

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

This is not used.

Gnito reviewed Dec 31, 2018

View reviewed changes

src/components/FieldArray/FieldArray.css Outdated

margin-bottom: 24px;

}

.field {

Copy link

Contributor

Gnito Dec 31, 2018

Choose a reason for hiding this comment

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

And this is not used either.

Gnito reviewed Dec 31, 2018

View reviewed changes

src/forms/PayoutDetailsForm/PayoutDetailsForm.js

/>

</div>

{values.accountType ? (

<React.Fragment>

Copy link

Contributor

Gnito Dec 31, 2018

Choose a reason for hiding this comment

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

👍

Gnito reviewed Dec 31, 2018

View reviewed changes

Gnito requested changes Dec 31, 2018

View reviewed changes

Gnito added review comments and removed review wanted labels Dec 31, 2018

OtterleyW force-pushed the add-stripe-business-accounts branch 2 times, most recently from 887b4de to 0a35c58 Compare January 2, 2019 13:18

OtterleyW force-pushed the add-stripe-business-accounts branch from e6f88a5 to be64212 Compare January 2, 2019 13:48

Gnito approved these changes Jan 2, 2019

View reviewed changes

Gnito added review ok and removed review comments labels Jan 2, 2019

OtterleyW force-pushed the add-stripe-business-accounts branch from be64212 to 40e2695 Compare January 2, 2019 14:42

OtterleyW temporarily deployed to sharetribe-starter-app January 2, 2019 14:46 Inactive

OtterleyW force-pushed the add-stripe-business-accounts branch 5 times, most recently from d2924ca to 2d64a5a Compare January 7, 2019 12:02

OtterleyW added 15 commits January 7, 2019 14:04

Add fields for company name and tax id

dd33c4d

Use radio buttons in PayoutDetailsForm

abe9733

Separate PayoutDetailsForm to PayoutDetailsFormIndividual and PayoutD…

1e5e70a

…etailsFormCompany

Create PayoutDetailsPersonalDetails subcomponent

a3400f4

Update PayoutDetailsAddress to contain section title

7dff833

Create PayoutDetailsBankDetails subcomponent

338fb79

Update user.duck.js to support also business accounts

a3b43ce

Add country specific tax id field labels

1ba857c

Add personal address to company account if required

6319c77

Create IconAdd

9d8d211

Add additional owners to company account if required

9c31f3e

Update IconClose so it can be used in smaller size

08eb9e3

Add new translation keys to fr.json

d0bc25f

Synchronize white modal width

ea22f94

Update changelog

d22da25

OtterleyW force-pushed the add-stripe-business-accounts branch from 2d64a5a to d22da25 Compare January 7, 2019 12:06

OtterleyW merged commit 400a0d3 into master Jan 7, 2019

OtterleyW deleted the add-stripe-business-accounts branch January 7, 2019 12:40

Gnito mentioned this pull request Jan 8, 2019

New release: v2.7.0 #993

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stripe business accounts #980

Add stripe business accounts #980

OtterleyW commented Dec 17, 2018 •

edited

Loading

Gnito Dec 31, 2018

Gnito Dec 31, 2018

Gnito Dec 31, 2018

Gnito Dec 31, 2018

Gnito Dec 31, 2018 •

edited

Loading

Gnito Dec 31, 2018

Gnito Dec 31, 2018

Gnito Dec 31, 2018

Gnito Dec 31, 2018

Gnito Dec 31, 2018 •

edited

Loading

Gnito left a comment

Gnito left a comment

Add stripe business accounts #980

Add stripe business accounts #980

Conversation

OtterleyW commented Dec 17, 2018 • edited Loading

Select individual or company account

Add additional owners

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

Gnito Dec 31, 2018 • 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

Gnito Dec 31, 2018 • edited Loading

Choose a reason for hiding this comment

Gnito left a comment

Choose a reason for hiding this comment

Gnito left a comment

Choose a reason for hiding this comment

OtterleyW commented Dec 17, 2018 •

edited

Loading

Gnito Dec 31, 2018 •

edited

Loading

Gnito Dec 31, 2018 •

edited

Loading