Skip to content

Implemented only one Interval on a page. #122

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

Closed
wants to merge 2 commits into from

Conversation

mweibel
Copy link

@mweibel mweibel commented Mar 28, 2013

This commit will fix jquery.timeago's issue to have an interval
for each timeago element on a page.
By doing this it also removes destroyed elements automatically.

As such, this commit fixes #96 and
#90.

Please share your comments. My code might not be the most idiomatic jQuery plugin code ever - please let me know what could be done better in your opinion, as well as let me know if there are any bugs.

Michael Weibel added 2 commits March 28, 2013 14:17
This commit will fix jquery.timeago's issue to have an interval
for each timeago element on a page.
By doing this it also removes destroyed elements automatically.

As such, this commit fixes rmm5t#96 and
rmm5t#90.
@pstadler
Copy link

+1

@mweibel
Copy link
Author

mweibel commented Oct 2, 2013

Just a quick update: Since I opened this pull request it has been in production at our website and it seems to work without any issues.
Wondering why this hasn't been merged yet nor got any comment.

@rmm5t
Copy link
Owner

rmm5t commented Oct 2, 2013

@mweibel, I'm sorry about that. I've been very busy trying to kick off a new company. I've done a poor job of shepherding the project lately. I really appreciate your passion and ambition for this.

Here are some reasons why I haven't been able to give this PR more time:

  1. It lacks tests. The timeago test suite needs some work as a whole, but I cannot pull in such a sweeping change without some coverage, especially to ensure some backwards compatibility.
  2. Timeago has changed quite a bit since this PR, and at a minimum, this PR needs a rebase and squash (i.e commits like 525a468 really shouldn't be in a PR, because they should have been squashed first). I haven't checked to see what kind of merge conflicts these changes bring right now.
  3. I haven't given it much thought yet, but on the surface, it looks like a lot of code for what it does. Maybe that's unavoidable, but Support unloading timeago elements. fixes #40 #90 looks more streamlined on the surface. I'm more apt to pull these things one small change at a time (i.e. auto-cleanup of removed elements then consolidating to one timer).
  4. In the back of my mind, I've been wanting to give timeago two things a) a new, cleaner test suite and b) A bit of a rewrite to incorporate the many open feature requests with a better underlying design to make future changes easier. That, of course, keeps getting postponed, and my fault.

In summary, I think the features and ideas behind this PR are excellent. I think the project as a whole needs some work first, and that's currently on me.

@mweibel
Copy link
Author

mweibel commented Oct 2, 2013

No worries, I know how hard it is sometimes to keep a project at life ;)

I'll take a closer look on your points but from a quick look I agree on them. I'll see whether I can fix those in a timely manner.

@mweibel mweibel closed this Mar 10, 2015
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.

Only use a single timer/setInterval
3 participants