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

568 | Quest Patient List View with configurable views #595

Merged
merged 35 commits into from
Oct 8, 2021
Merged

Conversation

maimoonak
Copy link
Contributor

@maimoonak maimoonak commented Oct 1, 2021

Fixes #568 - Depends on #586

Type
Choose one: Feature

Checklist

  • I have written Unit tests for any new feature(s) and edge cases for bug fixes
  • I have added any strings visible on UI components to the strings.xml file
  • I have updated the CHANGELOG.md file for any notable changes to the codebase
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the project's style guide
  • I have built and run the fhircore app to verify my change fixes the issue and/or does not break the app

@maimoonak maimoonak marked this pull request as ready for review October 4, 2021 08:46
Copy link
Collaborator

@ellykits ellykits left a comment

Choose a reason for hiding this comment

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

I have a couple of questions/comments:

  1. For the bottom navigation menu options how are we going to handle click actions?
  2. Is it possible to implement the bottom navigation menu options like the SideMenuOptions and have a click listener on each bottom menu option similer to the item count callback on SideMenuOptions?
  3. If step 2 is possible then I do not think we will need to pass the bottom navigation menu options to the register. We can however provide a configuration to toggle the visibility of the bottom navigation.
  4. Is it also possible to have the method as open in the BaseRegisterActivity class so it can be overridden in the subclass but we provide default implementation returning empty bottom navigation menu option.
  5. Is it also possible to rename the MenuOption to provide more context for instance NavigationMenuOption

@ellykits
Copy link
Collaborator

ellykits commented Oct 4, 2021

Is it also possible to replace the png icons with svg variants?

@ellykits
Copy link
Collaborator

ellykits commented Oct 4, 2021

Update the register button design and text

@maimoonak
Copy link
Contributor Author

Done! Incorporated the feedback

* WIP

* changelog updated

Co-authored-by: maimoonak <[email protected]>
@codecov
Copy link

codecov bot commented Oct 4, 2021

Codecov Report

Merging #595 (21e7039) into main (f62e925) will decrease coverage by 0.15%.
The diff coverage is 28.42%.

❗ Current head 21e7039 differs from pull request most recent head fe285c6. Consider uploading reports for the commit fe285c6 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main     #595      +/-   ##
============================================
- Coverage     36.28%   36.13%   -0.16%     
- Complexity      345      376      +31     
============================================
  Files           130      149      +19     
  Lines          3679     4140     +461     
  Branches        517      597      +80     
============================================
+ Hits           1335     1496     +161     
- Misses         2191     2463     +272     
- Partials        153      181      +28     
Flag Coverage Δ
anc 53.37% <55.55%> (-1.63%) ⬇️
eir 56.76% <40.00%> (-2.33%) ⬇️
engine 15.93% <22.17%> (+4.01%) ⬆️
quest 33.15% <31.51%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ore/anc/ui/anccare/register/AncRegisterActivity.kt 96.87% <ø> (ø)
...e/anc/ui/family/register/FamilyRegisterActivity.kt 96.55% <ø> (ø)
...eir/ui/patient/register/PatientRegisterActivity.kt 100.00% <ø> (ø)
...gine/configuration/app/ApplicationConfiguration.kt 0.00% <0.00%> (ø)
...ngine/configuration/app/ConfigurableApplication.kt 0.00% <ø> (ø)
...ne/configuration/view/RegisterViewConfiguration.kt 0.00% <0.00%> (ø)
...ata/remote/fhir/resource/FhirResourceDataSource.kt 0.00% <0.00%> (ø)
...e/data/remote/fhir/resource/FhirResourceService.kt 0.00% <0.00%> (ø)
...ircore/engine/ui/base/BaseMultiLanguageActivity.kt 0.00% <0.00%> (ø)
...ster/fhircore/engine/ui/login/BaseLoginActivity.kt 0.00% <0.00%> (ø)
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ae2f7e...fe285c6. Read the comment docs.

pld
pld previously approved these changes Oct 4, 2021
Copy link
Member

@pld pld left a comment

Choose a reason for hiding this comment

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

this looks good, please expand test coverage for the new code

@maimoonak
Copy link
Contributor Author

@ndegwamartin Unit test written for new code in Quest do not reflect in CI. I tried writing tests into engine but most of the classes need shadow application, and implementations for base-classes. Kindly look into it so that I can see the missing line after QuestTests are analyzed

pld
pld previously approved these changes Oct 6, 2021
Copy link
Member

@pld pld left a comment

Choose a reason for hiding this comment

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

lgtm!

@pld
Copy link
Member

pld commented Oct 6, 2021

@ndegwamartin Unit test written for new code in Quest do not reflect in CI. I tried writing tests into engine but most of the classes need shadow application, and implementations for base-classes. Kindly look into it so that I can see the missing line after QuestTests are analyzed

sorry @maimoonak I didn't realize that! changes approved, can someone else please take a look? And @ndegwamartin please merge if this gets another approval, thank you

pld
pld previously approved these changes Oct 8, 2021
@pld pld merged commit e38997d into main Oct 8, 2021
@pld pld deleted the 568-quest-list branch October 8, 2021 14:56
AbdulWahabMemon added a commit that referenced this pull request Oct 12, 2021
* main:
  Quest questionnaire sync (#590)
  567 | Registration handling - Resource update sync handling - Missing accou… (#618)
  568 | Quest Patient List View with configurable views (#595)
  Auth token issue fix and moving token to secure storage (#613)
AbdulWahabMemon added a commit that referenced this pull request Oct 12, 2021
* main:
  Quest questionnaire sync (#590)
  567 | Registration handling - Resource update sync handling - Missing accou… (#618)
  568 | Quest Patient List View with configurable views (#595)
  Auth token issue fix and moving token to secure storage (#613)
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.

Quest v0.1 / G6PD: Patient List View
5 participants