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

Proposal: Card isValid and handleSubmit props conflict #224

Open
peterszerzo opened this issue Nov 28, 2020 · 1 comment
Open

Proposal: Card isValid and handleSubmit props conflict #224

peterszerzo opened this issue Nov 28, 2020 · 1 comment
Assignees

Comments

@peterszerzo
Copy link
Contributor

peterszerzo commented Nov 28, 2020

In Card.tsx, there is both a handleSubmit and isValid props. The caller can provide contradictory props like { isValid: false, handleSubmit: () => { send(strangeValue); } }, where strangeValue can be a default causing unexpected outcomes, or a runtime error.

My proposal here would be to provide handleSubmit as an optional prop, and absence implicitly means invalidity.

On a practical level, this reduces implementations like this (parsedValue comes from the implementation of NumberInput, where public component parses the value of a text input):

<Card
  handleSubmit={() => {
    if (parsedValue !== null) {
      props.handleSubmit(parsedValue);
    }
  }}
  isValid={parsedValue !== null}
/>

to this (pardon the syntax here, could be written nicer with the callback pulled out top level):

<Card
  handleSubmit={parsedValue && (() => {
    if (parsedValue !== null) {
      props.handleSubmit(parsedValue);
    }
  })}
/>

This blog post is a much better explanation for all of this.

@peterszerzo peterszerzo self-assigned this Nov 28, 2020
@johnrees
Copy link
Contributor

Thanks Peter, this is sound advice. Honestly the Card stuff is very likely to change again in the next few days to accommodate Pages, it was just a quick refactoring that needed to be done as part of the process. That being said I'll take another proper look at this issue and the article you've shared with fresh eyes tomorrow 🙏

@johnrees johnrees self-assigned this Nov 30, 2020
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

No branches or pull requests

2 participants