Provide populationData source strings in app #597, Part1 #606
Conversation
…anged scenarios.py to include srcHospitalBeds in generating json
|
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/covid19-scenarios/covid19-scenarios/qwy1uvgkt |
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| srcHospitalBeds: | ||
| type: string | ||
| minLength: 1 | ||
|
|
There was a problem hiding this comment.
You are currently not setting srcHospitalBeds as required in the schema. This means the app will have to tolerate missing keys. I'm not sure if we should set it as required or not, we will need to see later.
There was a problem hiding this comment.
Yes, I wasn't sure if it was necessary to make it required or not given its tenuous inclusion at the moment, but it's only a 1 line change in code, so we can always account for it later. From my understanding (albeit rudimentary), not making it required wouldn't flag some sort of error in case the parameter isn't included. Given that we aren't deadset on this feature, I figured having that sort of flexibility wouldn't be too detrimental.
And understood. I certainly figured that the tsv's would be rather excessive if included.
To verify, is there some way I can edit my pull request to ensure those aren't included, or should I just be mindful in future commits to not include those files?
There was a problem hiding this comment.
I don't have a local checkout of this branch, so I think you should be safe to delete your last commit via resetting the head and force-pushing the branch. Or you just revert the commit (which keeps it in the history, with another undo-commit). In both cases, you can then make another commit on the branch which changes only the needed files.
There was a problem hiding this comment.
I believe the latest commit was properly reverted and should have only the necessary changes, but let me know if I didn't do it correctly.
|
Hi @realdy, please don't check in changed in From what I can tell, the other changes look good so far. As I noted, we will have to decide whether the src strings should be required by the schema, or be optional. |
noleti
added
the
pr: work in progress
…to explain functionality of files/help me remember which ones are likely relevant for the UI
…to explain functionality of files/help me remember which ones are likely relevant for the UI
…aram. Changed scenarios.py to include srcHospitalBeds in generating json" This reverts commit d512353.
…where the data comes from
Unnecessary Inclusion
| function getSrcStrings(srcHospitalBeds?: string, srcICUBeds?: string, srcPopulation?: string) { | ||
| let ret = ['Source: ', 'Source: ', 'Source '] | ||
| if (srcPopulation === undefined || srcPopulation === 'None') { | ||
| ret[0] = 'Source cannot be provided' |
There was a problem hiding this comment.
Why are the negative messages not all the same (provided vs found?) I would prefer something like 'No data source in our records.' or similar for all of them.
| @@ -43,7 +89,7 @@ function ScenarioCardPopulation({ errors, touched }: ScenarioCardPopulationProps | |||
| <FormSpinBox | |||
| identifier="population.populationServed" | |||
| label={t('Population')} | |||
| help={t('Number of people served by health care system.')} | |||
| help={t('Number of people served by health care system.\n'.concat(srcStr[0]))} | |||
There was a problem hiding this comment.
The '\n' does not seem to work, can you use a space instead?
| help={t( | ||
| 'Number of hospital beds available in health care system. Presets are rough estimates indicating total capacity. Number of beds available for COVID-19 treatment is likely much lower.', | ||
| 'Number of hospital beds available in health care system. Number of beds available for COVID-19 treatment is likely much lower.'.concat( |
There was a problem hiding this comment.
Can you use a space to separate the previous and new string?
|
@realdy This looks nice to me, thanks. I made some small comments, but overall this works well for me. @ivan-aksamentov I leave this to you to judge if this will complicate the app too much. I find this passing of source information through help text intuitive and less effort than a separate source page (which could still be made). We could even have source strings for the age distribution, imports per day (estimated by model.py imho), case-counts, etc (I leave this for the future). In general, links in the source strings are not clickable (and would be ideally). We can address that in the future, if this PR is generally accepted. |
…es for the help tag
|
Code Climate has analyzed commit dfe182d and detected 0 issues on this pull request. View more on Code Climate. |
|
@noleti |
noleti
removed
the
pr: work in progress
Related issues and PRs
Related to Issue #597
Changed scenarios.py to include srcHospitalBeds in generating json
Description
Review changes to ensure that initial changes for scenarios.py are sufficient and do not cause any severe issues before moving on to expressing the changes within the app
Impacted Areas in the application
scenarios.py
scenarios.json
scenarios.yml
type.py
Testing
Run "make all" and verify that there are no errors.
Inspect the generated scenarios.json to verify that srcHospitalBeds is included
Also inspect type.py to verify that "src_hospital_beds" is an include parameter