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

Required input attr #80

Closed
cezarsmpio opened this issue Jan 15, 2016 · 14 comments
Closed

Required input attr #80

cezarsmpio opened this issue Jan 15, 2016 · 14 comments

Comments

@cezarsmpio
Copy link

When I put required nothing happens

<Geosuggest
  placeholder="Start location"
  ref="start_location"
  required
/>
@ro-ka
Copy link
Contributor

ro-ka commented Jan 15, 2016

I suppose there should be a required attribute attached to the input, right?

@cezarsmpio
Copy link
Author

Yes, the result code would be:

<div class="geosuggest ">
    <input class="geosuggest__input " type="text" value="" placeholder="Start location" required>
    <ul class="geosuggest__suggests geosuggest__suggests--hidden"></ul>
</div>

You can use spread operator, something like <input {...this.props} />

@ro-ka
Copy link
Contributor

ro-ka commented Jan 15, 2016

Ah, okay, will add.

@cezarsmpio
Copy link
Author

Something like:

<input
  className={'geosuggest__input ' + this.props.inputClassName}
  ref="geosuggestInput"
  type="text"
  value={this.state.userInput}
  placeholder={this.props.placeholder}
  disabled={this.props.disabled}
  onKeyDown={this.onInputKeyDown}
  onChange={this.onInputChange}
  onFocus={this.onFocus}
  onBlur={this.hideSuggests}
  {...this.props}
/>

@ro-ka
Copy link
Contributor

ro-ka commented Jan 15, 2016

I wouldn’t put all the props on the element. I think a whitelist with supported attributes it the better way.

@cezarsmpio
Copy link
Author

You can create other prop like isRequired={true}, for example.

<Geosuggest isRequired />

@wedneyyuri
Copy link

@ro-ka Are you already working on this? I also need this feature. I can do a pull request for you.

@wedneyyuri
Copy link

@ro-ka What do you think about including other attributes like autocomplete, name, id, etc...

The solution proposed by @CezarLuiz0 seems to solve this problem, in my opinion the component should not know all the properties assigned to the input element.

@ro-ka
Copy link
Contributor

ro-ka commented Jan 18, 2016

Will add, yes. No time till now. :)
That’s true, but it’ll attach a lot of non standard attributes to the input element. Will a whitelist with attributes that are defined in the spec to add.

@ro-ka ro-ka closed this as completed in a43c388 Jan 18, 2016
@ro-ka
Copy link
Contributor

ro-ka commented Jan 18, 2016

Released as 1.15.0. Thanks for bringing this up!

@cezarsmpio
Copy link
Author

Awesome! 👍

@wedneyyuri
Copy link

@ro-ka I think it's missing the id attribute.

That's the case:

<label for="place">Search Places</label>

@ro-ka
Copy link
Contributor

ro-ka commented Jan 18, 2016

Totally, will fix that!

@ro-ka
Copy link
Contributor

ro-ka commented Jan 18, 2016

Fixed in 1.15.1. Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants