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 preexisting wrapper and custom class names #16805

Open
wants to merge 4 commits into
base: gh-pages
Choose a base branch
from

Conversation

TxHawks
Copy link
Contributor

@TxHawks TxHawks commented Dec 28, 2015

This PR adds two, somewhat related features:

  1. Custom classes:
    • Allows users to assign custom classes to the different element. this way, Awesomplete can be customize to work with users' existing css, instead of forcing them to add unneeded css to their codebase. For example, if a user already has a class of is-visually-hidden, there is no reason to duplicate that style for visually-hidden. Similarly, it is a likely use case that people using OOCSS style classes will want to assign classes to the ul.
  2. Pre existing wrapper element (based on @erquhart's pull request (allow users to create their own wrappers #16743) and resolves Doesn't work with Materialize UI #16718):
    • Some designs, e.g., Material Design forms, require that the input remains a sibling of other elements. This PR allows the user to wrap their input with a pre-existing element with a class of their definition (awesomplete-wrapper by default).
    • If the input does not have such a wrapper an initialization time, one will be created be Awesomplete, just like before, albeit with awesomplete-wrapper or the user-defined class.

CSS, documentation and test updated to represent chnages.

Fix test so that it expect the input's wrapper to have a
class of "awesomplete-wrapper"
className: "awesomplete",
around: input
});
this.container = $("." + this.containerClass, this.input.parentNode) ||
Copy link
Owner

Choose a reason for hiding this comment

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

If the point is allowing a custom element for this, why not accept an element instead of a class? Or a selector, at the very least? Also, how will an element be around input and still be a child of its parent? Unless I’m missing something, that seems impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ehm, no not really...

#16718 is about having a structure where an element must remain a sibling of the input, like in many material-design implementations.

right now, if we have

<div class="some-wrapper-class">
  <input id="search" class="awesomplete someOtherClass" />
  <label for="search">Search for something</label>
</div>

Awesomplete will wrap the input with a div, separating it from from the label:

<div class="some-wrapper-class">
  <div class="awesomplete">
    <input id="search" class="awesomplete someOtherClass" />
  </div>
  <label for="search">Search for something</label>
</div>

What's needed to resolve #16718 is for new Awesomeplete('.awsomplete', {containerClass: 'some-wrapper-class'} to not wrap the input - because it is already wrapped by some-wrapper-class - leaving us with the original structure, so that the input and the label remain siblings.

This, inherently, also allows for custom elements, as it just looks at the existing html to see if it can be left as is, or needs modifying, based on a class name. new Awesomplete should just leave the following as is, allowing for the wrapper to be an element that isn't a div:

<label class="awesomplete">
  <input id="search" class="awesomplete someOtherClass" />
  Search for something
</label>

@LeaVerou
Copy link
Owner

Hello there! Thanks for contributing!
I’m a bit uncomfortable with the CSS changes. These would break existing stylesheets written for Awesomplete before this PR was merged.
I would prefer it if the class was added additionally.

@TxHawks
Copy link
Contributor Author

TxHawks commented Dec 30, 2015

Done

@fractaledmind
Copy link

This conflicts, at least in general purpose, with #16787, which sparked a discussion on the relation of Awesomplete to Bliss. The web of interconnected issues here likely means this PR (and that one) need to wait a bit before some of the underlying issues are made concrete.

@krlicmuhamed
Copy link

Any news?

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.

Doesn't work with Materialize UI
4 participants