Skip to content
This repository was archived by the owner on Apr 21, 2025. It is now read-only.

Comments

Restore symptom checker#1935

Merged
patniemeyer merged 17 commits intomasterfrom
restore-symptom-checker
Feb 28, 2021
Merged

Restore symptom checker#1935
patniemeyer merged 17 commits intomasterfrom
restore-symptom-checker

Conversation

@patniemeyer
Copy link
Contributor

This replaces the health check "poster" tab on the home page with the restored symptom checker code from commit: d614c40 and does what is necessary to get it working within the current app.

All lint issues have been fixed and I believe this is working, not withstanding one known issue: that our application styled button does not show the disabled state. I will open an issue for this separately.

Additionally the updated code does not utilize the observable pattern and possibly other app conventions adopted after it was written. We can open issues for those going forward.

I have tested this in the simulator.

Copy link
Contributor

@theswerd theswerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@brunobowden brunobowden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patniemeyer - great to get this running again. Main comment is that we'll want to keep around the symptom poster for countries where we can't get the check up tool approved. We should import both of them in to the app and then allow toggling between them using the yaml config.

My concern is that this may require additional work so that the nav bar icon for "Check-Up" can be controlled by the home_index.en.yaml file and be configured. Since that's the case, let's deal with that in another PR and just focus on fixing up the other comments before landing this one.

@stale
Copy link

stale bot commented Feb 17, 2021

This item has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the resolved:stale No recent activity on the issue or PR label Feb 17, 2021
@stale stale bot removed the resolved:stale No recent activity on the issue or PR label Feb 20, 2021
…vern whether the symptom poster page or symptom checker are shown in the check-up tab on the home page.
@brunobowden
Copy link
Contributor

@patniemeyer - this is great. My one thought was that show_symptom_checker would be better as a property of the symptom_checker.en.yaml file and shouldn't be something that the top level app needs to know about. So the idea was that the main page would link to the Check-Up tool and then based on the symptom_checker.en.yaml config, it would either show the symptom page or the symptom checker tool. That has better encapsulation but I'm wary about it being a lot of refactoring work.

@patniemeyer
Copy link
Contributor Author

@brunobowden Yes, that makes sense. I will move it.

@brunobowden
Copy link
Contributor

brunobowden commented Feb 21, 2021 via email

…d yaml with alternate views based on content.
@patniemeyer
Copy link
Contributor Author

As suggested I have merged the two pages into one CheckUpPage driven by the symptom_checker yaml. I have tried to follow the ContentWidget pattern found in the app here but something is wrong: It seems that if the widget is built while content is null there is nothing to trigger a rebuild and the page remains empty until something down the line triggers a refresh. It looks like the mobx observable stuff is supposed to cause the content widget to rebuild automatically... but this isn't working for some reason? I'm wondering if any of you are familiar with this code and see something obvious that I'm doing wrong...

@patniemeyer patniemeyer merged commit b8cc9b2 into master Feb 28, 2021
@patniemeyer patniemeyer deleted the restore-symptom-checker branch February 28, 2021 23:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants