Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Implemented only one Interval on a page. #122

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants

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 rmm5t/jquery-timeago#96 and
rmm5t/jquery-timeago#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 some commits Mar 28, 2013

Michael Weibel Implemented only one Interval on a page.
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/jquery-timeago#96 and
rmm5t/jquery-timeago#90.
7891957
Michael Weibel Reset default refreshMillis again (done for debugging purposes 525a468

+1

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.

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 #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 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