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

Local countdown #12

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

Local countdown #12

wants to merge 2 commits into from

Conversation

p0358
Copy link
Contributor

@p0358 p0358 commented Nov 29, 2017

  • Add local countdown if departure is close enough (so we get a smooth second-by-second cooldown before the next refresh)
  • Fix ttss_base if project is not installed in web root (a dot for relative path)
  • Fix in map.js for green border if tram is already on stop/loop (as it was done in previous pull request in index.js, didn't notice the one in map.js)

Please review.

@jacekkow
Copy link
Owner

I would like to see such changes:

  • do not call loadTimes inside setInterval function - this may cause weird behavior if many trains should arrive in succession (eg. in 2, 4 and 6 seconds - it may result in three calls each canceling the preceding one),
  • IMHO the way to go should be to keep references to <tr> and relevant <td> elements along with actualRelativeTime (maybe use td.dataset),
  • go through all the elements every second and only update text content (plus className), optionally on-demand (ie. compare what is generated with current value and setText if required - but I am not sure how much of a performance hit will incur from not doing it, if any). This would also make should_activate_smooth_timer obsolete.

This would lower both number of modifications and additional code required - pretty much only function for setting status text and proper className should be extracted and used).

As you have stated - updating half of the DOM tree every second is no good.

It would also be unnecessary DOM updating every second without any visual change.

Thanks for the contribution!

Update from original repo
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