-
Notifications
You must be signed in to change notification settings - Fork 5
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
HAPP-1597: Rewrites lab test details in compose #529
base: feature/Android-UI-2.0
Are you sure you want to change the base?
HAPP-1597: Rewrites lab test details in compose #529
Conversation
- Uses Card component - Receives modifier - Uses Arrangement spacedBy
- updated recommendation screen UI
- upgraded gradle version to 8.1 - upgraded java version 17 - removed popBackstack from BaseFragment and also removed its reference usage. - removed composeEmail from BaseFragment to FragmentExtensions.kt - updated HomeScreen floe handling to lifecycle aware. - updated UI to handle remove action click - updated authentication status observation
- added ability to see recommendations for primary and dependent user.
- fixed click to expand issue - fixed color of the count issue - fixed ordering issue - fixed title center issue on tool bar.
- removed icon from the title - fixed issue with order by date
- added feature to remove tile from the home screen - added ability to manage tiles from manage tile option - added local tile management feature
|
||
override fun getCommentEntryTypeCode() = CommentEntryTypeCode.LAB_RESULTS | ||
val uiState = viewModel.uiState.collectAsState().value |
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.
Please use collectAsStateWithLifecycle instead of collectAsState.
Reference: https://medium.com/androiddevelopers/consuming-flows-safely-in-jetpack-compose-cde014d0d5a3
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.
done
override fun getCommentView(): AddCommentLayout = binding.comment | ||
override fun onResume() { | ||
super.onResume() | ||
viewModel.getLabTestDetails(args.labOrderId) |
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.
This should be called from LabTestScreen.
navigationAction = { findNavController().popBackStack() } | ||
) { | ||
LabTestScreen( | ||
uiState = uiState, |
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.
Should pass viewModel inside LabTestScreen instead of UiState.
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.
done
if (state.onError) { | ||
showError() | ||
viewModel.resetUiState() | ||
observeWork(SYNC_COMMENTS) { |
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.
Should start observing worker from composable scree.
This will help to reduce usage of fragment and migration to compose easier.
|
||
handleNoInternetConnection(state) | ||
getComments(state.parentEntryId) | ||
private fun onSubmitComment(content: String) { |
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.
This should be called from compose screen
} | ||
} | ||
|
||
private fun getTestResult( |
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.
This should be done in viewModel.
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.
done
} | ||
|
||
@Composable | ||
private fun LabTestResultItemUi(labTestDetail: LabTestDetail) { |
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.
I would suggest to have this in its own separate file with its own preview.
It should stay in the same package. if we find a use case of reuse we will take this to the component package
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.
done
} | ||
|
||
@Composable | ||
private fun LabTestItemUi(title: Int?, body: String?) { |
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.
I would suggest to have this in its own separate file with its own preview.
It should stay in the same package. if we find a use case of reuse we will take this to the component package
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.
done
} | ||
|
||
body?.let { | ||
MyHealthClickableText( |
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.
I would recommend usage of HGTextButton
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.
This is not a button, it's a long Text that contains a clickable link, like Spannable on TextViews
@@ -7,5 +7,8 @@ import androidx.core.text.HtmlCompat | |||
fun String?.orPlaceholder(placeholder: String = "--"): String = | |||
this ?: placeholder | |||
|
|||
fun String?.orPlaceholderIfNullOrBlank(placeholder: String = "--"): String = |
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.
This is duplicate of the above method declared String?.orPlaceholder
and also has un-nacessary if condition check.
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.
They are actually different: orPlaceholder
will use placeholder only if the content is null, orPlaceholderIfNullOrBlank
will also return the placeholder if the content is blank
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
7dd8b12
to
5c6ca29
Compare
No description provided.