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

Added destroy lifecycle method #24

Closed
wants to merge 2 commits into from
Closed

Added destroy lifecycle method #24

wants to merge 2 commits into from

Conversation

kmalakoff
Copy link
Contributor

With single page apps, you may want to manage the lifecycle of the DOM more precisely. I have added a pull request for a destroy method that can be used to clean up the events when pages change.

@zenorocha
Copy link
Owner

Hey @kmalakoff, that would be a good addition.

Here are some things that could be improved:

  • According to delegate-events docs, the 3rd param is a callback so we don't need that when unbinding.
  • Destroy method should be added on ClipboardAction too and Clipboard should call it.

@kmalakoff
Copy link
Contributor Author

All of those sound good assuming one instance of Clipboard is managed as a group.

FYI: I originally tried to use ClipboardAction directly (understanding the event delegation tradeoff) given #25 since it wold be easier to integrate with React so definitely a destroy on the ClipboardAction could be good to expose a lower-level API. That said, after multiple attempts, I couldn't get it to work so settled with the jQuery approach using Clipboard.

@zenorocha zenorocha added the p1 label Oct 1, 2015
@zenorocha
Copy link
Owner

Just started reviewing :)

:octocat: Sent from GH.

@zenorocha
Copy link
Owner

Thank you, pull request merged!

:octocat: Sent from GH.

zenorocha added a commit that referenced this pull request Oct 4, 2015
zenorocha added a commit that referenced this pull request Oct 4, 2015
zenorocha added a commit that referenced this pull request Oct 4, 2015
zenorocha added a commit that referenced this pull request Oct 4, 2015
@zenorocha
Copy link
Owner

Merged, tested and documented. Would you mind checking the final result and leave your opinions? Thanks for contributing @kmalakoff.

@zenorocha zenorocha closed this Oct 4, 2015
@kmalakoff
Copy link
Contributor Author

Yeah, works well. Thanks!

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