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

bugfix(issue #14): add in destroy method #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chrisrng
Copy link
Contributor

Fixes issue #14

@ghost
Copy link

ghost commented Jun 22, 2016

Hi @chrisrng , can we add an example to the Demo product to demonstrate how to use the .destroy?


```javascript
var memoryLeak = new Slider(document.getElementById('leakingSlider'));
memoryLeak.destroy(true);
Copy link

Choose a reason for hiding this comment

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

Non-blocking question, @chrisrng , can we expand the .destroy accepted parameter to accept as object instead of a boolean flag? Perhaps define some good names as accepted object keys to be expressive by native object key names, and the values of the object keys would be your choice of appropriate data type, i.e.:

memoryLeak.destroy({
  isCompleteDestroy: true
});

or

memoryLeak.destroy({
  isDestroyRefrencesOnly: true
});

Think if one sets up the accepted parameter with object with meaningful named object keys to control the .destroy API, it would pave the scalability of the .destroy API, if the .destroy API ever expands.

@jimmyangel
Copy link

Any plans to merge this PR into master? Thank you.

@chrisrng
Copy link
Contributor Author

chrisrng commented Jun 5, 2018

Hey @jimmyangel ! Unfortunately I no longer work at Priceline, left almost 2 years ago :)

I think this PR is good to go tho ... 2 years ago haha

@Emuentes
Copy link

@BeniCheni @jxnblk
Is there any reason that @chrisrng's PR was never merged? Just curious ;-)

@jxnblk
Copy link

jxnblk commented Jun 26, 2018

Sorry, we don't have any active maintainers for this project. It should probably be archived and clearly stated in the readme

@joshbDev
Copy link
Contributor

fwiw I think @chrisrng and myself would be more than happy to be maintainers for the project

@Emuentes
Copy link

Well the flat-line is a good indicator. I hadn't thought about whether it was in active development or not ;-)

@joshbDev @chrisrng maybe it's time for a "based on" spin-off ;-)

flat-lined

@BeniCheni
Copy link
Contributor

Think if anyone is interested in becoming maintainer(s) of omni-slider, perhaps the interested maintainer(s) can work with @ pricelinelabs to fork the omni-slider repo out of the @ pricelinelabs & transfer the authorship and maintainer roles & responsibility out of @ pricelinelabs.

Echoing @ jxnblk's previous comment, @ pricelinelabs doesn't have any active maintainer(s) to support omni-slider.

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.

7 participants