[Refactor:Notifications] Individual Notification Component#12137
[Refactor:Notifications] Individual Notification Component#12137
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12137 +/- ##
=========================================
Coverage 21.68% 21.68%
Complexity 9598 9598
=========================================
Files 268 268
Lines 36620 36620
Branches 475 475
=========================================
Hits 7941 7941
Misses 28208 28208
Partials 471 471
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Rkoester47
left a comment
There was a problem hiding this comment.
I tested the homepage notifications for this branch, and everything seems to be working properly. I tested: mark as seen, clicking on the notification itself, and clicking on the course hyperlink. All of it functioned properly in both Firefox and Edge, in light and dark themes. The changes you've made to the code seem to have left the homepage notifications in proper working order.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
lavalleeale
left a comment
There was a problem hiding this comment.
Splitting looks good, and local testing showed functionality to be the same
RyanStyron
left a comment
There was a problem hiding this comment.
Great refactor without affecting functionality.
Why is this Change Important & Necessary?
HomePage.vuehas a component,AllNotificationsDisplay.vue, that functions as the entire notification panel for that page.As more things are added to this file (such as new buttons when we implement it for the Course Notifications page), it is becoming large (~400 lines), and we losing the benefits of the component-based approach that Vue offers.
What is the New Behavior?
Essentially, this PR splits up the
AllNotificationsDisplay.vuefile. Instead of looping through and displaying divs defined in that file for each individual notification, it now loops through and displays a component,Notification, for each notification. All styling and notification-specific logic has been moved to the new file,Notification.vue, dividing the original file into two ~200 line files.This change just abstracts logic to improve maintainability. This may help visualize the it (think of these are Vue components):
Before:
After:
What steps should a reviewer take to reproduce or test the bug or new feature?
Ensure that the functionality of the notifications panel on the home page is unchanged.
Automated Testing & Documentation
There are no automated tests for the notifications system, see #11908.
Other information
This is not a breaking change. I intend to create a follow-up PR to this to convert the Course Notifications page. By breaking the existing code into multiple components, it will be easier to migrate, and maintain.
There is potential to add even more components beyond this, such as one for the header, but these are subject for discussion. I think the individual notifications is the most important for now.