browser-extension icon indicating copy to clipboard operation
browser-extension copied to clipboard

Refactor and change workings of handleLiveChanges.js

Open emilgoldsmith opened this issue 8 years ago • 1 comments

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

emilgoldsmith avatar Mar 22 '17 02:03 emilgoldsmith

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.

martynchamberlin avatar Mar 24 '17 03:03 martynchamberlin