aatishb / covidtrends

Tracking the growth of COVID-19 Cases worldwide
https://aatishb.com/covidtrends/
MIT License
301 stars 107 forks source link

Auto-update country URL #179

Closed MrSpiffyClean closed 4 years ago

MrSpiffyClean commented 4 years ago

Proposed solution for #163.

The submitted code does some things:

I've tested the code locally (using the simple Python server) and everything seems to work properly, although any sort of code review is very welcome (as I'm still a bit new to JS).

There is an odd case whenever someone selects all countries and then changes to Reported Deaths, instead of showing the simplified parameter, the URL bar does clog up a little. The end result is the same.(Fixed)

Feedback is welcome.

netlify[bot] commented 4 years ago

Deploy preview for frosty-benz-8c81bf ready!

Built without sensitive environment variables with commit 09f74c9c8b4d8df49ca933ed26fcfe0f56fadec0

https://deploy-preview-179--frosty-benz-8c81bf.netlify.app

rpkoller commented 4 years ago

out of curiosity is it necessary that the question mark on the initial page load (with the default values) is shown? that url looks a bit odd ;) https://deploy-preview-179--frosty-benz-8c81bf.netlify.app/? aside function wise it looks fine and works as expected (at least from my perspective but i am not js savvy ;))) )

MrSpiffyClean commented 4 years ago

You are right, it does look a bit odd. I might have seen it while testing but was never bothered by it. Anyway, I've managed to track down the fault and do a quick fix, it should work as expected now.

rpkoller commented 4 years ago

yep I can confirm, the ? is gone for the default view now :) cool

aatishb commented 4 years ago

Wow, @MrSpiffyClean this is a big one! Thanks for submitting this, I'll aim to review by Tuesday. @rpkoller thanks for testing & feedback!

MrSpiffyClean commented 4 years ago

I've just added a bit more stuff, aiming to fix the edge case I pointed out at the start of the pull request. By doing it, I ended up clearing another edge case.