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

Where does the attributes class and id should go? #16

Closed
davidmh opened this issue Feb 28, 2014 · 10 comments
Closed

Where does the attributes class and id should go? #16

davidmh opened this issue Feb 28, 2014 · 10 comments

Comments

@davidmh
Copy link
Collaborator

davidmh commented Feb 28, 2014

Currently, they are rendered in the .autocomplete div, but I was thinking that maybe they could be more useful in the input within the .autocomplete.

Case in point:

<autocomplete ... attr-class="form-control"></autocomplete>

renders:

<div class="autocomplete form-control">
  <input ...>
</div>

wrong element

Working with bootstrap or another css framework requires adding the class to the input itself. Currently there's no way of doing that.

TL;DR: move the class and id attributes to the input inside .autocomplete

@azzamallow
Copy link
Contributor

  • 1 for this. Ive run into the exact same problem.

the "autocomplete" class on that div is specific enough to customise elements within, a way to add class on the input field would be much handier.

@JustGoscha
Copy link
Owner

Okay, maybe'it would be good if the class would be on both? On the input and the outer div

@chesleybrown
Copy link
Collaborator

That wouldn't make sense to me... couldn't you just target the input if the outer div had a class?

autocomplete > input

@azzamallow
Copy link
Contributor

@JustGoscha sounds fine.

@chesleybrown you can do that, the issue here is bootstrap has classes which we would like to apply to the input field

@davidmh
Copy link
Collaborator Author

davidmh commented Mar 5, 2014

@chesleybrown Yes, but it's not optimal.

This is DRY-er and stays synchronised with the styles in bootstrap:

<input class="form-control" ... />
<input class="form-control input-lg" ... />
<!--- N other custom inputs -->

Against this:

.autocomplete input {
  /* all the properties in .form-control */
}

.different .autocomplete input {
  /* overrides such as .input-lg */
}

/* N other customisations */
<input ... />
<input ... />
<!--- N other inputs -->

Which it's a bit harder to maintain and doesn't .


How about this?

<autocomplete
  attr-class="wrapper-class"
  attr-id="wrapper-id"
  attr-input-class="form-control input-lg"
  attr-input-id="input-id"
  ...
 ></autocomplete>

@azzamallow
Copy link
Contributor

@davidmh, @chesleybrown, @JustGoscha

The wrapping div already has 'autocomplete' on there as a class. Do you have an example where using attr-class on that wrapping div to add a second class is advantageous?

@davidmh
Copy link
Collaborator Author

davidmh commented Mar 5, 2014

Nop, it's just to make it backwards compatible. Maybe someone already deployed something using this directive.

Those attributes could be marked as deprecated to warn about future deletion or simply removed. They are not really relevant anymore.

Edit:
And attr-input- seems a bit more explicit.

@JustGoscha
Copy link
Owner

@davidmh Ok, let's do it, just make sure it's still optional an works without 😉
Also update the documentation in the README.md, whoever makes the change 😃

@azzamallow
Copy link
Contributor

I'm dependant on a fix so ill go ahead and do it

@azzamallow
Copy link
Contributor

See: #20

Once verified could you plus update the bower version so I cna start using it?

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

4 participants