-
Notifications
You must be signed in to change notification settings - Fork 43
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
Create from dom object #7
Create from dom object #7
Conversation
1 similar comment
src/dual-listbox.js
Outdated
this.select = selector; | ||
} else { | ||
this.select = document.querySelector(selector); | ||
} | ||
|
||
this.selected = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to have the variable declarations at the top.
This is merged in 1.0.6. Thank you for contributing. @piotr-gawron Can you also add yourself to the contributers? |
@JostCrow Thanks, What do you mean by add myself to contributers? |
src/dual-listbox.js
Outdated
@@ -19,7 +19,11 @@ const SELECTED_MODIFIER = 'dual-listbox__item--selected'; | |||
class DualListbox { | |||
constructor(selector, options={}) { | |||
this.setDefaults(); | |||
this.select = document.querySelector(selector); | |||
if (this.isElement(selector)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we do selector instanceof Element
here? Or is there any particular reason you want do a custom check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it depends how many browsers (and which versions) you want to support: https://stackoverflow.com/questions/384286/javascript-isdom-how-do-you-check-if-a-javascript-object-is-a-dom-object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We support latest + previous of all major browsers + IE11. Just checked IE11 which seems to support this. I think @JostCrow is working on setting up cross env testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#6 This is a setup of what we want to cover. (we still need to add all the browsers. Also we want to support some mobile browsers.) IE11 will be dropped when it is no longer supported by Microsoft.
src/dual-listbox.js
Outdated
* @Private | ||
* Returns true if argument is a DOM node | ||
*/ | ||
isNode(o) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function being used?
n.v.m. already removed...
@piotr-gawron If you like, you can add your name to the |
This PR provides simple functionality that allows to create DualListbox from pure DOM element.
It's usefull when the page is created dynamically via JS and some elements might be attached to the document later (or at completely different part of the code).
As an alternative to check if argument to the constructor is a dom element I considered checking if argument is a string.
Here are some links for reference: