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.

Sign up for GitHub

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

Jump to bottom

Repeated Group Headers #1994

Merged
merged 16 commits into from
Jun 10, 2024
Merged

Repeated Group Headers #1994

merged 16 commits into from
Jun 10, 2024

Conversation

kevinmost
Copy link
Contributor

@kevinmost kevinmost commented May 3, 2023 •
edited
Loading

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #726

Description
Add headers to repeated groups.

The header shows the index of the repeated group instance, plus a delete button that removes that entire instance.

Type
Choose one: Feature

Screenshots (if applicable)

Screenshot

Checklist

  • I have read and acknowledged the Code of conduct.
  • I have read the Contributing page.
  • I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
  • I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
  • I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
  • I have built and run the demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

@kevinmost kevinmost marked this pull request as ready for review May 3, 2023 15:32
@kevinmost kevinmost force-pushed the km/repeatedGroupsHeaderUi branch 2 times, most recently from 2ab9012 to f601489 Compare May 4, 2023 15:31
@jingtang10
Copy link
Collaborator

@shelaghm please take a look at the screenshot!

we didn't really know what to do with the heading so used cardinal numbers 1,2,...

@kevinmost and i did wonder if there's a group header extension from the FHIR spec we can use... or maybe we can use a placeholder like Group 1, Group 2,...

jingtang10
jingtang10 requested changes May 11, 2023
View reviewed changes
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

great pr! love this ❤️. thanks @kevinmost

datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireEditAdapter.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireViewItem.kt Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireEditAdapter.kt Show resolved Hide resolved
...va/com/google/android/fhir/datacapture/views/factories/QuestionnaireItemViewHolderFactory.kt Show resolved Hide resolved
datacapture/src/main/res/layout/repeated_group_instance_header_view.xml Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireAdapterItem.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireEditAdapter.kt Outdated Show resolved Hide resolved
...e/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponses.kt Outdated Show resolved Hide resolved
@shelaghm
Copy link

Looks amazing! @kevinmost
A few suggestions to make it even better:

  1. Add 'Group' in front of 1, 2. So it reads Group 1, Group 2, etc.
  2. Move the sample date question text so that it is the placeholder text inside the text field and dropdown.
  3. Use the EntryFormat / hint text field for the date format.

(See screenshot for details)
Repeat Group 12

@kevinmost kevinmost requested a review from a team as a code owner May 31, 2023 12:22
@kevinmost
Copy link
Contributor Author

Updated

jingtang10
jingtang10 requested changes Aug 2, 2023
View reviewed changes
Copy link
Collaborator

@jingtang10 jingtang10 left a comment

Choose a reason for hiding this comment

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

thanks for this @kevinmost!

happy to approve once all comments have been addressed - don't think i have any more comments.

datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireAdapterItem.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireAdapterItem.kt Show resolved Hide resolved
...oogle/android/fhir/datacapture/contrib/views/PhoneNumberViewHolderFactoryInstrumentedTest.kt Outdated Show resolved Hide resolved
...oogle/android/fhir/datacapture/contrib/views/PhoneNumberViewHolderFactoryInstrumentedTest.kt Outdated Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireAdapterItem.kt Show resolved Hide resolved
datacapture/src/main/res/layout/repeated_group_instance_header_view.xml Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireEditAdapter.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt Show resolved Hide resolved
datacapture/src/test/java/com/google/android/fhir/datacapture/QuestionnaireViewModelTest.kt Show resolved Hide resolved
datacapture/src/main/java/com/google/android/fhir/datacapture/extensions/MoreViews.kt Outdated Show resolved Hide resolved
@FikriMilano FikriMilano mentioned this pull request Sep 21, 2023
Open
jingtang10
jingtang10 requested changes May 31, 2024
View reviewed changes
...t/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireItemComponentsTest.kt Outdated Show resolved Hide resolved
...t/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireItemComponentsTest.kt Outdated Show resolved Hide resolved
...t/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireItemComponentsTest.kt Outdated Show resolved Hide resolved
...oogle/android/fhir/datacapture/contrib/views/PhoneNumberViewHolderFactoryInstrumentedTest.kt Show resolved Hide resolved
...src/androidTest/java/com/google/android/fhir/datacapture/test/QuestionnaireUiEspressoTest.kt Outdated Show resolved Hide resolved
...src/androidTest/java/com/google/android/fhir/datacapture/test/QuestionnaireUiEspressoTest.kt Outdated Show resolved Hide resolved
@santosh-pingle
Copy link
Collaborator

santosh-pingle commented Jun 4, 2024 •
edited
Loading

https://github.com/google/android-fhir/pull/1994/files#diff-1f4ba456c28c5400e29ae89383faf8d1a8d9109c80299bc0efb1ed3cf11818b2R834

This change has side effects:

  1. Nested items of repeated group items are not shown.
  2. Answers that are captured in questionnaireResponse.json and passed as an argument to QuestionnaireFragment do not show nested items of repeated group items.

If the following fix is added, then the above-mentioned issues are not seen:
add(questionnaireResponseItem.item)

@jingtang10

@jingtang10 jingtang10 enabled auto-merge (squash) June 10, 2024 13:57
@jingtang10 jingtang10 merged commit 1016558 into google:master Jun 10, 2024
4 checks passed
@allan-on allan-on mentioned this pull request Jun 14, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jingtang10 jingtang10 jingtang10 approved these changes

@santosh-pingle santosh-pingle Awaiting requested review from santosh-pingle santosh-pingle is a code owner

@omarismail94 omarismail94 Awaiting requested review from omarismail94 omarismail94 was automatically assigned from google/android-fhir

@MJ1998 MJ1998 Awaiting requested review from MJ1998 MJ1998 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
Android FHIR SDK
Status: Complete
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle repeated groups
5 participants
@kevinmost @jingtang10 @shelaghm @santosh-pingle @android-fhir-bot