-
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
Notification inline actions - preliminary refactor #20172
Notification inline actions - preliminary refactor #20172
Conversation
The `onBindViewHolder` function is already to complex, and too long. The rule suppression is still necessary even after this change, but this refactoring is a step toward removing the suppression.
…fications-share-an-own-published-post
This is mostly empty for now (only implementing a stub for the `Like` type), however it is the starting point to add the remaining notification types declaratively.
Generated by 🚫 Danger |
📲 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.
|
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.
Thanks for refactoring the code, this is helpful to me!
Code looks great, and the testing result is as expected.
Thank you for reviewing and testing! |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/notifications_refresh_p1 #20172 +/- ##
====================================================================
- Coverage 40.19% 40.18% -0.02%
====================================================================
Files 1468 1469 +1
Lines 67560 67579 +19
Branches 11185 11187 +2
====================================================================
Hits 27157 27157
- Misses 37914 37933 +19
Partials 2489 2489 ☔ View full report in Codecov by Sentry. |
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 refactoring this part of the code @mkevins 🙇
The code looks good and I haven't noticed any behavior changes in my tests 🎉
Description
This PR is a preliminary step to refactor and align shared elements of the design for inline actions on notifications. In the development of resolving #20040, a strategy emerged that would benefit other similar issues / tasks: establishing a declarative model for
Notification
s, and treating access to the pre-existing class(es) as an implementation detail. This allows us to treat the existing code structure itself as "immutable", a benefit since any changes there may be hard to follow in the current state.This PR also adds many missing nullability annotations, and extracts some logic from a method which violates lint for
"CyclomaticComplexMethod"
and"LongMethod"
rules (though more work is needed before the rule suppression can be removed).To Test:
Regression Notes
Notifications
Manual testing
Additional refactoring is necessary before automated tests can be added
PR Submission Checklist:
RELEASE-NOTES.txt
if necessary.Testing Checklist: