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

Refactor sharedProps prop in src/forms/CurrentExpenses.js #986

Open
turnerhayes opened this issue Nov 11, 2018 · 3 comments
Open

Refactor sharedProps prop in src/forms/CurrentExpenses.js #986

turnerhayes opened this issue Nov 11, 2018 · 3 comments
Labels
discussion needed tech-debt Used for issues that typically don't directly affect what the end user sees, such as coding styles

Comments

@turnerhayes
Copy link
Collaborator

#965 uses a prop called sharedProps for several components; this should be separated out into the specific props each component needs.

@turnerhayes turnerhayes added the tech-debt Used for issues that typically don't directly affect what the end user sees, such as coding styles label Nov 11, 2018
@turnerhayes turnerhayes added this to To do in 2:3 Refactoring: Components via automation Nov 11, 2018
@knod
Copy link
Collaborator

knod commented Nov 11, 2018

Could you add an example of how it would look where it's separated?

@turnerhayes
Copy link
Collaborator Author

For example, the <Under13> component takes a sharedProps parameter. The format of that appears to be (looking where it's used):

{
    timeState:         current,
    type:              type,
    time:              time,
    updateClientValue: updateClientValue,
}

So instead of having the Under13 component be declared as

const Under13 = function ({ snippets, type, sharedProps, current, updateClientValue }) {
...
}

it could be

const Under13 = function ({ snippets, type, timeState, time, current, updateClientValue }) {
...
}

The code that uses <Under13> could do

 <Under13
          snippets          = { snippets }
          type              = { type }
          current           = { current }
          updateClientValue = { updateClientValue }
          { ...sharedProps } />

The advantage of this approach is that it's clearer what Under13 expects as its props, rather than having to inspect the code to see what ends up getting passed to it. Does that make sense?

@knod
Copy link
Collaborator

knod commented Nov 12, 2018

That's strange that updateClientValue and type are redundant in the shared props. I'll definitely think about it and other folks can as well, and look at how all those properties are being used in the code. I can see discussing some pros of a sharedProps object if it's used well, but this may or may not be the place for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed tech-debt Used for issues that typically don't directly affect what the end user sees, such as coding styles
Projects
Development

No branches or pull requests

2 participants