-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
The inline action of Like Comment #20187
The inline action of Like Comment #20187
Conversation
…-inline-action # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.java
…-inline-action # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt
…-inline-action # Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt # WordPress/src/main/res/layout/notifications_list_item.xml
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/notifications_refresh_p1 #20187 +/- ##
====================================================================
- Coverage 40.15% 40.14% -0.01%
====================================================================
Files 1469 1469
Lines 67683 67697 +14
Branches 11211 11210 -1
====================================================================
Hits 27179 27179
- Misses 38010 38024 +14
Partials 2494 2494 ☔ View full report in Codecov by Sentry. |
Generated by 🚫 Danger |
WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/notifications/adapters/NotesAdapter.kt
Outdated
Show resolved
Hide resolved
Thank you, @antonis Nice catch! I found that not only the gray color, but also the green color are different in light/dark mode. |
WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListViewModel.kt
Outdated
Show resolved
Hide resolved
@@ -27,30 +30,26 @@ import javax.inject.Named | |||
class NotificationsListViewModel @Inject constructor( | |||
@Named(UI_THREAD) mainDispatcher: CoroutineDispatcher, |
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 could add a background dispatcher and use it in the "like coroutine" to avoid blocking the UI when updating the like status.
@Named(BG_THREAD) val bgDispatcher: CoroutineDispatcher
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'm gonna replace @Named(UI_THREAD) mainDispatcher: CoroutineDispatcher
with @Named(BG_THREAD) val bgDispatcher: CoroutineDispatcher
to see if the blocking issue in main thread can be resolved.
But I guess the root cause is not the about the dispatcher, it should throw NetworkOnMainThreadException
if I run on the wrong thread.
I noticed that UI refreshes many times because of the event bus, I'd like to remove redundant events after #20024 is done.
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.
Makes sense @jarvislin 👍
I noticed that UI refreshes many times because of the event bus, I'd like to remove redundant events after #20024 is done.
Sounds good. We can merge this PR and tackle this later.
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.
Hmmm the main thread is blocking even I don't click on the action icon. So there should be a different root cause.
WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsListFragmentPage.kt
Show resolved
Hide resolved
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.
Great work @jarvislin 👍
I've tested the implementation on a Pixel 5/Android 14 and it worked as described in most cases for me. The code also looks good.
I've added a few comments inline with suggestions.
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.
Thank you for the changes @jarvislin 🙇
All look good to me 🎉
We can iterate on the UI update issue later #20187 (comment)
Part of #20024
This is the feature for liking a comment from the inline action.
This PR contains:
To Test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist: