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

feat: add autoComplete to most inputs / fields. #696

Merged
merged 4 commits into from
Apr 23, 2019

Conversation

montezume
Copy link
Contributor

@montezume montezume commented Apr 18, 2019

Summary

Adds prop autoComplete to fields and inputs.

@montezume montezume requested review from emmenko and tdeekens and removed request for emmenko April 18, 2019 12:40
@montezume montezume self-assigned this Apr 18, 2019
Copy link
Contributor

@tdeekens tdeekens left a comment

Choose a reason for hiding this comment

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

What if we just allow on and off while defaulting to off or on?

@montezume
Copy link
Contributor Author

@tdeekens but modern browsers accept a whole whack of values for autocomplete...
https://stackoverflow.com/questions/34712926/how-browsers-store-data-for-autocomplete-and-where/34776932#34776932

@@ -96,7 +96,8 @@ storiesOf('Examples|Forms/Fields', module)
errors={formik.errors.animal}
isRequired={true}
touched={formik.touched.animal}
name="animal"
name="name"
isSearchable={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch

@jonnybel
Copy link
Contributor

This is the type of prop that I'd personally prefer to spread directly to the HTML element in a remainingProps fashion, because we're basically just adding more code, tests and documentation for stuff that is already native to the HTML, and seems very unnecessary. (Other examples are props like isDisabled, readOnly, autoFocus, etc, as well as mouse events and whatnot)

But I know this would require a complete refactor of all inputs/fields (and even other components in our UI-kit), so I just wanted to state this for consideration.

That being said, nothing wrong with this PR in particular considering how we're doing things so far 😄

@montezume
Copy link
Contributor Author

@jonnybel really good point. It just adds noise to the component and especially to the readme the way it is now...

@montezume
Copy link
Contributor Author

@jonnybel do you want to open an issue and list the props that should be whitelisted for this spreading behaviour? I guess aria- props, etc...

@montezume montezume merged commit f1274ec into master Apr 23, 2019
@montezume montezume deleted the ml-support-autocomplete-props branch April 23, 2019 08:33
@jonnybel
Copy link
Contributor

Yes, I can do that.

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.

3 participants