-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add more jank tracking #1833
base: main
Are you sure you want to change the base?
Add more jank tracking #1833
Conversation
Change-Id: I5e6186ddc488230f5c0c75db4b38efe458e39e12
@@ -304,7 +310,7 @@ private fun SearchResultBody( | |||
verticalItemSpacing = 24.dp, | |||
modifier = Modifier | |||
.fillMaxSize() | |||
.testTag("search:newsResources"), | |||
.testTag(testTag), |
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.
You should update NavigationTest
, also why change it?
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.
You should update NavigationTest
No test dependents were found on search:newsResources
.
also why change it?
The composable name is SearchResultBody
, but the test tag was incorrectly set to search:newsResources
. To align with the correct naming convention, the test tag has been updated to search:searchResult
. This ensures consistency and clarity in the codebase.
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.
Thought I had seen search:newsResources
somewhere in test, you right.
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.
Let me know if we should handle the test tag naming improvements in a separate PR.
@@ -211,7 +212,8 @@ fun EmptySearchResultBody( | |||
horizontalAlignment = Alignment.CenterHorizontally, | |||
modifier = Modifier.padding(horizontal = 48.dp), | |||
) { | |||
val message = stringResource(id = searchR.string.feature_search_result_not_found, searchQuery) | |||
val message = | |||
stringResource(id = searchR.string.feature_search_result_not_found, searchQuery) |
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.
Keep the original formatting to avoid reviewing unchanged code
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.
Change-Id: I1be6dfd557a4d798f82c761c89686cd3268989a6
Hi @tobioyelekan, just a gentle reminder to re-review this PR when you have time. I've addressed the previous comments, and it's ready for another look. Thanks! |
Hi LGTM, I'll let the maintainers of the project also take a look |
df3e99d
to
64e3256
Compare
Hi @alexvanyo , could you review this PR when you get a chance? Let me know if there's anything that needs adjustment. Thanks! |
Track jank while scrolling anything that's scrollable.