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

Allow us to override input type #578

Closed
gfpacheco opened this issue May 22, 2020 · 6 comments · Fixed by #580
Closed

Allow us to override input type #578

gfpacheco opened this issue May 22, 2020 · 6 comments · Fixed by #580

Comments

@gfpacheco
Copy link
Contributor

Previously I had a component that used TextField internally, I was happy to be able to just tell him its type by sending a prop:

// ...somewhere in my component
<TextField type={type} />

But with the release of 0.7.0 you're overriding my prop and always setting type="text" anytime I use TextInput.

I get that you wanted to create different components for different types, and even if I think that's unnecessary I respect that. But let me override the type if I want, otherwise I'll have to keep switching between different components depending on the type of the input.

Does it work if I just move this line just after type={type}?

Would you approve if I opened that PR?

@peterp
Copy link
Contributor

peterp commented May 22, 2020

Thanks for brining this to our attention @gfpacheco. We would 100% accept a PR that introduces that!

You could move {...tagProps} all the way to the bottom so that everything is overrideable.

@cannikin
Copy link
Member

I was going to suggest creating a new generic <InputField> where it is expected that you set the type. Seems kind of weird to explicitly say <TextField> and then set type="number".

@gfpacheco
Copy link
Contributor Author

I like that!

But that doesn't change my mind about users being able to override any default prop, does it make sense?

@thedavidprice
Copy link
Contributor

I was going to suggest creating a new generic where it is expected that you set the type. Seems kind of weird to explicitly say and then set type="number".

^^ If we don't have it already, this is a much better long-term solution. But given all the new field types with v0.7.0 (here in docs), I guess the new <NumberField> would be best in this case, for example, correct? (@gfpacheco You're probably already ahead of me here, but wanted to make sure you also saw the additions to Forms in v0.7)

@gfpacheco
Copy link
Contributor Author

gfpacheco commented May 22, 2020

Yes, I did see them! And it's great that we have these single-purpose components, but in my particular case my CustomComponent gets the type prop and I just wanted my component to pass it to its own TextField as a prop as well. A generic InputField would work just as fine, though.

And again, I still think letting the user override any default props is always a good thing.

@thedavidprice
Copy link
Contributor

@gfpacheco Ah, completely understood. Thanks for the reply and, again, for the input + help here!

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 a pull request may close this issue.

4 participants