-
Notifications
You must be signed in to change notification settings - Fork 296
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
Check if previous/next page is hidden to show previous/next buttons #2569
Check if previous/next page is hidden to show previous/next buttons #2569
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to include contiguous set of items not with page extension into another page.
"If there are items at the same level as a 'page' group that are listed before the 'page' group, they will be treated as a separate page"
Reference - https://build.fhir.org/ig/HL7/fhir-extensions//CodeSystem-questionnaire-item-control.html
So if the questionnaire looks like -
{
itemWithNoPageExtension1,
itemWithPageExtension2, // have nested items
itemWithPageExtension3,
itemWithNoPageExtension4,
itemWithNoPageExtension5,
itemWithNoPageExtension6,
itemWithPageExtension7,
}
The pages would look like following:-
{
page1: {
itemWithNoPageExtension1,
},
page2: {
itemWithPageExtension2
},
page3: {
itemWithPageExtension3
},
page4: {
itemWithNoPageExtension4,
itemWithNoPageExtension5,
itemWithNoPageExtension6
},
page5: {
itemWithPageExtension7
}
}
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt
Outdated
Show resolved
Hide resolved
Ah okay, makes sense. Probably we'd then need to make changes to our questionnaires, thanks |
@LZRS does this change handle the following use case?
|
Yes, so far the changes in this PR handle that but the ongoing discussions were on whether it should be hidden, since it's also treated as a 'page', or if we should instead handle it by throwing an exception |
@LZRS In the second page, which also has a page extension, the previous button needs to be hidden. Can this change handle that? |
Yeah, it handles that |
Here's some updates about the issue in dealing with pages, by @MJ1998 |
@MJ1998 @jingtang10 have you decided whether we should make the implementation more relaxed? |
@FikriMilano @MJ1998 Reviewing the current existing implementation, Sibling items to a page extension group are treated as page even though they may not be of type
The spec has also been updated
IMO, it feels the current implementation is already relaxed and wouldn't necessarily crash In the PR, I have just updated to check that the previous or next button is not hidden, to show the previous/next button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Approving with one small NIT.
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #2568
Description
Change
getQuestionnairePages
to only return questionnaire pages from QuestionnaireItems that have extensionAlternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?
Type
Bug fix
Screenshots (if applicable)
Checklist
./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the style guide of this project../gradlew check
and./gradlew connectedCheck
to test my changes locally.