Skip to content

Conversation

pfiller
Copy link
Contributor

@pfiller pfiller commented Jul 30, 2013

@harvesthq/chosen-developers

This is a slight modification of #1414 from @koenpunt. It just takes the outerHTML method he created and moves it into the AbstractChosen method. At some point, we should create a class of dom utilities, but this is an ok home for now.

Fixes #336

@kenearley
Copy link

Looks good. :shipit:

pfiller added a commit that referenced this pull request Jul 30, 2013
Use document.createElement instead of html string
@pfiller pfiller merged commit e118518 into master Jul 30, 2013
@pfiller pfiller deleted the koenpunt-dom-create-element branch July 30, 2013 21:14
@pfiller
Copy link
Contributor Author

pfiller commented Jul 30, 2013

Thanks @koenpunt!!

@koenpunt
Copy link
Contributor

I still think that outerHTML shouldn't be an instance method of AbstractChosen, as it is in no way related to the object.. Why do you want it to be an instance method?
As utility method it is also only available in the same scope as the classes, so not exposed to the global (window) scope.

@pfiller
Copy link
Contributor Author

pfiller commented Jul 31, 2013

I agree with you that outerHTML has no business in AbstractChosen, but I also don't like the idea of a called method that doesn't tell a story about where it came from.

outerHTML(some_el);
this.outerHTML(some_el);
ChosenUtils.outerHTML(some_el);

The first option looks like a native JS method. When it comes time to debug it's output, that could add a little bit of unnecessary confusion. The second two tell you exactly where they came from and tracking down the source of the function is a no brainer.

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.

images for chosen doesnt work on firefox
3 participants