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

Separation of duties #47

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

igorlima
Copy link
Contributor

@igorlima igorlima commented Apr 22, 2019

So... in essence... this PR divides the business logic from the view of the component. Why? By doing so, it allows another React component to extend the main class, and renders a different view.

Several projects do not take advantages of style-components, it means this limits other projects to import that great library, because it forces others to use style-components. For instance, to import a new framework in a big project, such as style-components, it takes sometime to work between architecture and design teams. So, by having two classes, it makes possible to render another view without the need to install style-component. See example below - another library extending from CreditCardUtilities:

image

At the end, this PR makes possible to have different React components working in harmony. See others sample below:

Sample A Sample B
image A image B

@jxom
Copy link
Contributor

jxom commented Apr 22, 2019

Nice! I think separating the business logic from the view of the component is a good idea. However, I think the abstraction may need to be thought out a little more.

My main concern is that the 'utility' component assumes the children input props are set up properly (e.g. correct refs, handlers, etc). I think there should be a way to 'fetch' the correct props from the utility component to attach to the custom inputs.

My other concern is allowing the 'view' component to extend the 'utility' component. Extending other components are commonly considered an anti-pattern in React due to shared namespaces & the user is forced to use a class component.

I'd recommend either: converting the 'utility' component into a React Hook, or using the 'render prop' pattern. This will not force the user to using a class component to implement their input fields, and will have clear understanding of where the utility functions are coming from.

Here could be a possible React Hook example (pretty high level):

import { useCreditCardInputs } from 'react-credit-card-input';

export default function CreditCardInputs() {
  const { cardNumberProps, expiryProps, cvcProps } = useCreditCardInputs();
  
  return (
    <div>
      <input name="card-number" {...cardNumberProps} />
      <input name="expiry" {...expiryProps} />
      <input name="cvc" {...cvcProps} />
    </div>
  )
}

and maybe a render props example:

import { CreditCardInputsContainer } from 'react-credit-card-input';

export default function CreditCardInputs() {
  return (
    <CreditCardInputsContainer>
      {({ cardNumberProps, expiryProps, cvcProps }) => (
        <div>
          <input name="card-number" {...cardNumberProps} />
          <input name="expiry" {...expiryProps} />
          <input name="cvc" {...cvcProps} />
        </div>
      )}
    </CreditCardInputsContainer>
  )
}

@igorlima igorlima changed the title Separation of duties WIP: Separation of duties Apr 28, 2019
@igorlima igorlima force-pushed the medipass/separation-of-duties branch from 1d84d1d to 5402f78 Compare April 28, 2019 22:36
Fix an issue on showing card image
@igorlima igorlima force-pushed the medipass/separation-of-duties branch from f6819e0 to 3892403 Compare April 28, 2019 23:29
@igorlima
Copy link
Contributor Author

igorlima commented Apr 28, 2019

not sure if that change handles all concerns - at least, there's no extension from the 'utility' component anymore. Below is an example how it looks like now on...

The PR was marked as WIP - Working In Progress - there are some other minor things to tackle later.

Let me know if there's any thing else missing - would be great to have some feedback...

<Container>
  <FieldWrapper>
    <CardImage />
    <InputWrapper>
      <CardNumberInput/>
    </InputWrapper>
    <InputWrapper>
      <CardExpiryInput />
    </InputWrapper>
    <InputWrapper>
      <CardCvcInput />
    </InputWrapper>
    <InputWrapper>
      <CardZipInput />
    </InputWrapper>
  </FieldWrapper>
</Container>

@igorlima igorlima changed the title WIP: Separation of duties Separation of duties May 6, 2019
@igorlima
Copy link
Contributor Author

igorlima commented May 6, 2019

@jxom the business logic is separated from the view of the component by using the React Hook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants