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

add destroy method #44

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add destroy method #44

wants to merge 4 commits into from

Conversation

baptistebriel
Copy link

Hi,
Related to #43 & #23, I tried to add a destroy method to the Select class, and also clean up a bit the event listeners. You'll see that I edited the welcome demo with a hacky setTimeout, just to see what's done on destroy.

Basically, I just created an object with all the eventListener named functions. There's surely some enhancement to do, so we could skip the const self = this thing...

this.evnts = {
  target: {
    click() {},
    focus() {},
    blur({relatedTarget}) {},
    deviceClick(e) {}
  },
  drop: {}
  // etc...
};

So we can remove the event listeners in the destroy function.
I had a question tho; why not directly using the class to define the init method instead of doing this?

Select.init = (options={}) => {}

I've moved it inside the class. Let me know your thoughts!

@codeclown codeclown mentioned this pull request Jan 27, 2016
@silvenon
Copy link

👍 Why does it say that you want to merge this from an unknown repository?

@baptistebriel
Copy link
Author

Sorry for the delay @silvenon — it says that I want to merge this from an unknown repository because I deleted my fork; I finally ended up using a custom function instead of select because of major performances issues (tether/issues/148)

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.

2 participants