Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send form refactor proposal - Closes #2269 #2310

Merged
merged 1 commit into from Aug 7, 2019

Conversation

@slaweet
Copy link
Member

commented Aug 2, 2019

What issue have I solved?

#2269

How have I implemented/fixed it?

This is a PR for Spike ticket, so it contains only the proposal for the refactoring. There is a separate ticket for the implementation #2159

How has this been tested?

There are no changes that need to be tested.

Review checklist

@slaweet slaweet self-assigned this Aug 2, 2019

@slaweet slaweet requested review from osvaldovega and massao Aug 2, 2019

// - FormLSK - Contains all LSK-specific functionality (message, static fee) and renders FormBase
// - FormBTC - Contains all BTC-specific functionality (dynamic fee selection) and renders FormBase
// - FormBase - Contains all functionality common to LSK and BTC (Box, address input,
// amount input, confirm button)

This comment has been minimized.

Copy link
@osvaldovega

osvaldovega Aug 3, 2019

Contributor

@slaweet I understand the component for FormLSK and FormBTC but the FormBase looks like the the parent component and then I don't understand the FormWrapper.

If I understand right will be something like.

  • FormBase
    • FormWrapper
      • FormLSK/FormBTC

In any case I think that most important is the FormLSK and FormBTC what will be very good for the send.

This comment has been minimized.

Copy link
@slaweet

slaweet Aug 5, 2019

Author Member

What I meant is something like this (very simplified):

const FormWrapper = ({ activeToken, ...props }) => (
  activeToken === 'BTC'
    ? <FormBTC {...props} />
    : <FormLSK {...props} />
);
const FormLSK = (props) => (
  <FormBase {...props} >
    <StaticFee />
    <MessageInput />
  </FormBase>
);
const FormBTC = (props) => (
  <FormBase {...props} >
    <DynamicFee />
  </FormBase>
);
const FormBase = ({ children }) => (
  <Box>
    <AddressInput />
    <AmountInput />
     {children}
   <ActionButtons />
  </Box>
);

I used a similar pattern in Request https://github.com/LiskHQ/lisk-hub/blob/development/src/components/request but there, RequestWrapper corresponds to FormBase in this proposal and index.js(Request) is proposed FormWrapper. So I would change the naming here to be consistent with the request.

This comment has been minimized.

Copy link
@reyraa

reyraa Aug 6, 2019

Member

I'd go with dividing the components based on their purpose rather than the values they use. An amount input, is gonna validate the amount and show the fiat equivalent not matter the token. the difference will only be the pure validation function.
Same goes for recipient input. Address validation and fetching the bookmarks is exactly the same. I'd suggest not duplicate this functionality in two components (FormBTC and FormLSK). Instead, better have it central with a pure validation function dedicated for each. I assume with proper architecture, fetching bookmarks must be no different other than passing different parameters to the same function.
On thing is to decide to show or hide the Message, which is as simple as it can get.

This comment has been minimized.

Copy link
@slaweet

slaweet Aug 6, 2019

Author Member

@reyraa I think you miss-read what I proposed above.

AddressInput and AmountInput is in the FormBase component, so it's common for BTC and LSK.

What is not described above is how they get the token-specific validation functions. Token-specific component can easily pass a token-specific function for that into the FormBase, like validateAddress below:

const FormBase = ({ children, validateAddress }) => (
  <Box>
    <AddressInput validateAddress={validateAddress} />
    <AmountInput />
     {children}
   <ActionButtons />
  </Box>
);

This comment has been minimized.

Copy link
@slaweet

slaweet Aug 6, 2019

Author Member

And also I didn't mean that AddressInput and AmountInput necessarily need to be separate components (depends on their size). I just put them to illustrate what functionality will be in which component.

This comment has been minimized.

Copy link
@reyraa

reyraa Aug 6, 2019

Member

Ok, I got you now. I agree with what proposed then.

This comment has been minimized.

Copy link
@yasharAyari

yasharAyari Aug 7, 2019

Member

I think it is better to have on parent form and define the differences inside the form component like AddressInput and AmountInput. Then you can move the validation methods to a utility.

@slaweet slaweet requested review from yasharAyari and reyraa Aug 5, 2019

@massao

massao approved these changes Aug 5, 2019

Copy link
Contributor

left a comment

💯

@reyraa

reyraa approved these changes Aug 6, 2019

@slaweet slaweet merged commit 7b11a66 into development Aug 7, 2019

4 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
coverage/coveralls Coverage remained the same at 94.765%
Details

@slaweet slaweet deleted the 2269-send-form-refactor-proposal branch Aug 7, 2019

@slaweet slaweet added the ready label Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.