Skip to content

Allow preexisting wrapper and custom class names - #16805

Open
TxHawks wants to merge 4 commits into
nudeui:gh-pagesfrom
TxHawks:feature/existing-wrapper-and-custom-classes
Open

Allow preexisting wrapper and custom class names#16805
TxHawks wants to merge 4 commits into
nudeui:gh-pagesfrom
TxHawks:feature/existing-wrapper-and-custom-classes

Conversation

@TxHawks

@TxHawks TxHawks commented Dec 28, 2015

Copy link
Copy Markdown
Contributor

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"
Comment thread awesomplete.js

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Collaborator

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

TxHawks commented Dec 30, 2015

Copy link
Copy Markdown
Contributor Author

Done

@fractaledmind

Copy link
Copy Markdown

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
Copy Markdown

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