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

Refactor and change workings of handleLiveChanges.js #93

Open
emilgoldsmith opened this issue Mar 22, 2017 · 1 comment
Open

Refactor and change workings of handleLiveChanges.js #93

emilgoldsmith opened this issue Mar 22, 2017 · 1 comment

Comments

@emilgoldsmith
Copy link
Contributor

  • I believe the eventListener could be on DOMContentLoaded instead of on load
  • I think the notifications code could be more specific instead of relying on indices? Maybe also add ids there or use some other more specific safe way of fetching that information? Too easy to break if Chess.com changes the html layout a tiny bit
  • Do we need the delay for the getting notifications? What exactly is it that does? And will it run if you don't stay on chess.com for at least 60 seconds?
  • In regards to Remove the page reload when pressing reset #80 there might be a small edge case where you start changing the styles before it has changed initial styles into inline styles, i think changing the eventListener to DOMContentLoaded could definitely make this a much smaller if not non-existent edge case, and/or make buttons disabled until chess.com is loaded and we switched in the inline styling.
@martynchamberlin
Copy link
Member

I think the notifications code could be more specific instead of relying on indices?

Agreed!

Do we need the delay for the getting notifications?

Wow. Looking at the code it looks like there's a 61 second delay before we get notification count. That's definitely an oversight. Instead, we should be getting notifications 1 second (that gives the Chess.com JS a minute to boot), then have an setInterval (or recurring setTimeout) every 60 seconds afterwards. Good catch.

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

No branches or pull requests

2 participants