Skip to content
This repository was archived by the owner on Jan 20, 2022. It is now read-only.

Small text field fixes #67

Merged
merged 4 commits into from
Aug 15, 2019
Merged

Small text field fixes #67

merged 4 commits into from
Aug 15, 2019

Conversation

jgzuke
Copy link
Contributor

@jgzuke jgzuke commented Aug 13, 2019

Make label optional again

Im not sure why this went away, adding it back since it seemed like an unintentional change to make it required.

Proptypes extends InputHTMLAttributes

Im not sure when this went away either, but without it you have to spread props to avoid type errors which isn't very ergonomic, for example

{...{
  name: 'password',
  type: 'password',
}}

instead of

name="password"
type="password"

@justinanastos
Copy link
Contributor

I don't want to spread all the props; I want this to be a container with well-defined options. I also want to find a better solution than needing to add .opt-out to our inputs.

@jgzuke
Copy link
Contributor Author

jgzuke commented Aug 14, 2019

@justinanastos sounds good, I switched to just the name and type since they tend to be very common on inputs

Copy link
Contributor

@justinanastos justinanastos left a comment

Choose a reason for hiding this comment

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

Looks good.

This failed CI because you didn't add a semver label. Please read the README (https://github.com/apollographql/space-kit#releases) to see why this matters.

Also explained in the readme in that same section, the title of this PR is what's going to show in the changelog. If the title isn't enough to explain, then add a ## release notes section, again, per the readme.

@jgzuke jgzuke added the minor Increment the minor version when merged label Aug 15, 2019
@justinanastos justinanastos self-requested a review August 15, 2019 00:16
@jgzuke jgzuke merged commit 87912af into master Aug 15, 2019
@jgzuke jgzuke deleted the jgzuke/text-field-fixes branch August 15, 2019 00:18
@justinanastos
Copy link
Contributor

🚀 PR was released in v1.5.0 🚀

@justinanastos justinanastos added the released This issue/pull request has been released. label Aug 15, 2019
@justinanastos
Copy link
Contributor

I just realized this PR adds the name prop to the component and doesn't actually use it. Yet another reason why I need to start adding tests.

@jgzuke
Copy link
Contributor Author

jgzuke commented Aug 22, 2019

@justinanastos unfortunately they still got passed through in otherProps. Im not sure what kind of test would catch that since it was still working properly (even though it should certainly have been explicit). We could write a lint rule to say all explicitly typed props should be destructured to avoid that kind of mistake, im not sure what the use cases for exceptions to that rule would be?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
minor Increment the minor version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants