Async getQuestionnairePages and Add progress bar to the 'Next' button #2645
Conversation
FikriMilano
requested review from
aditya-07,
jingtang10,
MJ1998 and
santosh-pingle
as code owners
- With unmerged PR #9 - WUP PR google#2178 - WUP google#2652 - WUP google#2521 - WUP google#2645 - WUP google#2648 - WUP google#2650
…ub.com:opensrp/android-fhir into 2639-async-process-for-get-questionnaire-pages
|
I haven't build the app from your change yet, but it looks like you're adding a new component. Would it be possible for you to do this instead: https://m3.material.io/components/progress-indicators/guidelines#01dea24a-ce29-4353-99da-35472e24f4f9 to combine the circular progress indicator with the next button? |
|
@jingtang10 yeah, that's exactly what I did |
| viewModelScope.launch(Dispatchers.IO) { | ||
| var isReferenced = false | ||
| kotlin.run { | ||
| isReferenced = questionnaireItem.isExpressionReferencedBy(questionnaire) | ||
| if (isReferenced) return@run | ||
|
|
||
| questionnaire.item.flattened().forEach { item -> | ||
| isReferenced = questionnaireItem.isEnableWhenReferencedBy(item) | ||
| if (isReferenced) return@run | ||
|
|
||
| isReferenced = questionnaireItem.isExpressionReferencedBy(item) | ||
| if (isReferenced) return@run | ||
| } | ||
| } | ||
| if (isReferenced) isLoadingNextPage.value = true | ||
| modificationCount.update { it + 1 } | ||
|
|
||
| updateAnswerWithAffectedCalculatedExpression(questionnaireItem) | ||
|
|
||
| modificationCount.update { it + 1 } | ||
| updateAnswerWithAffectedCalculatedExpression(questionnaireItem) | ||
| pages = getQuestionnairePages() | ||
| isLoadingNextPage.value = false | ||
| modificationCount.update { it + 1 } | ||
| } |
There was a problem hiding this comment.
We should investigate if parallel coroutine here could cause any issues incase of answersChangedCallback in quick successions like typing an answer.
There was a problem hiding this comment.
will investigate
| isReferenced = questionnaireItem.isExpressionReferencedBy(questionnaire) | ||
| if (isReferenced) return@run | ||
|
|
||
| questionnaire.item.flattened().forEach { item -> |
There was a problem hiding this comment.
Is there a way to avoid this for loop? If the questionnaire is long? Also is there any value in avoiding it? Maybe we can just time this part and see how long it takes given a large questionnaire with many expressions and enable whens. If it takes < x% of the time then we don't really need to worry about it.
There was a problem hiding this comment.
go through only:
- pages
- do it once
consider to use dependency graph, in the future.
| var isReferenced = false | ||
| kotlin.run { | ||
| isReferenced = questionnaireItem.isExpressionReferencedBy(questionnaire) | ||
| if (isReferenced) return@run | ||
|
|
||
| questionnaire.item.flattened().forEach { item -> | ||
| isReferenced = questionnaireItem.isEnableWhenReferencedBy(item) | ||
| if (isReferenced) return@run | ||
|
|
||
| isReferenced = questionnaireItem.isExpressionReferencedBy(item) | ||
| if (isReferenced) return@run | ||
| } | ||
| } | ||
| if (isReferenced) isLoadingNextPage.value = true |
There was a problem hiding this comment.
this whole bit should be calculated once for the questionnaire instead of being calculated each time when the answer changes?
| @@ -29,4 +29,6 @@ data class QuestionnaireNavigationUIState( | |||
| val navSubmit: QuestionnaireNavigationViewUIState = QuestionnaireNavigationViewUIState.Hidden, | |||
| val navCancel: QuestionnaireNavigationViewUIState = QuestionnaireNavigationViewUIState.Hidden, | |||
| val navReview: QuestionnaireNavigationViewUIState = QuestionnaireNavigationViewUIState.Hidden, | |||
| val navNextProgressBar: QuestionnaireNavigationViewUIState = | |||
| QuestionnaireNavigationViewUIState.Hidden, | |||
| ) | |||
There was a problem hiding this comment.
@jingtang10 what do you think of this current code?
Initially I did add a new UI state for loading, but changed my mind, then implement it this current way.
My reasoning:
In the future, I'm thinking there might be other types of UI other than button that might use this UI state, it makes sense to have Loading state for button, but might not be for other types of UI.
Or am I just overthinking it(?)
There was a problem hiding this comment.
Jing:
Use loading state to update both next button and next button's circular progress indicator together
This SDK PRs are included: google/android-fhir#2645 google/android-fhir#2663
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #2639
Description
Clear and concise code change description.
Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?
Type
Feature
Screenshots (if applicable)
Checklist
./gradlew spotlessApplyand./gradlew spotlessCheckto check my code follows the style guide of this project../gradlew checkand./gradlew connectedCheckto test my changes locally.