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

Add hospital line + fix bugs & lint issues #259

Merged
merged 3 commits into from Mar 25, 2020

Conversation

wolverdude
Copy link
Contributor

@wolverdude wolverdude commented Mar 25, 2020

Fixes #226 by using preliminary hospital bed data from here

Once #125 / #258 is completed, the code will need slight updates to incorporate the new data, but it won't break in the meantime.

There are a couple of things I didn't do that might be an issue though:

  • The data transforms the actual hospital bed capacity into "capacity to handle COVID patients assuming that 5% of cases require hospitalization". This is not made clear anywhere on the frontend.
  • I assumed the data in beds_p_100k was equivalent to "unused critical care capacity" rather than "total critical care capacity". If it's the latter, that's a problem. We need the former in order for the charts not to be deceptive.

This PR also fixes an inefficient double-rendering upon page load and a number of smaller lint issues with the JavaScript.

Screen Shot 2020-03-24 at 5 22 10 PM

@lagerros
Copy link
Contributor

lagerros commented Mar 25, 2020

Stylistic feedback:

  • Could the line be thicker? (not super visible atm, and think it's thinner than the others)
  • Could the legend text be separated from the other legend texts somehow? (This might be harder, but would it even be hovering right above the line?)

Overall looks good, and in the interests of moving fast I'll approve this and you can merge, and then you can add the changes in another PR.

@wolverdude
Copy link
Contributor Author

wolverdude commented Mar 25, 2020

@lagerros I added two points to the PR description that I want your thoughts on. With regard to your points:

  1. Yes, I intentionally made the line thinner because it helped distinguish that line from the others, but I can put it back to the same thickness as the other lines.
  2. I seriously doubt it's feasible to customize the legend this way with Plotly. It can probably be done under the hood with D3, but I doubt that's worth it. I also think it would be undesirable to begin with. People are used to seeing one, single legend on a chart, and sharding it might be confusing.

@wolverdude
Copy link
Contributor Author

New line thickness:

Screen Shot 2020-03-24 at 6 25 38 PM

@wolverdude wolverdude merged commit d9d3a2f into master Mar 25, 2020
@wolverdude wolverdude deleted the wolverdude/hospital-line branch March 25, 2020 01:39
@hnykda
Copy link
Contributor

hnykda commented Mar 25, 2020

Looks good! Followup and confusion explanation #264 .

Re separation of the line: I personally think it's actually better, but because I simply think about this as a part of the chart/traces, which may be different from the user 🤷‍♂️ . Nevertheless, it's surely possible to add custom text at the chart via annotations.

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.

use data for hospital view in the chart
3 participants