[Refactor:Notifications] Convert Course Notifications to Vue#12159
[Refactor:Notifications] Convert Course Notifications to Vue#12159
Conversation
Created CourseNotifications.vue, which is the page that the NotificationsDisplay.vue component will sit on. I will now add arguments for the component for mark all seen and settings.
Working on the padding between both
Logic in the notifications panel will be based on the course boolean. Trying to make mark_seen work on the notifications page.
…noti-component
There were duplicate styles since we were loading the old notification.css file. Added logic so we don't display course on course notifications page. Now that styling is complete, the next commits will be the logic.
Synced up buttons, added mark seen functionality, and added notification links
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12159 +/- ##
=========================================
Coverage 21.71% 21.71%
Complexity 9612 9612
=========================================
Files 268 268
Lines 36073 36070 -3
Branches 475 475
=========================================
Hits 7832 7832
+ Misses 27770 27767 -3
Partials 471 471
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Fixed shadows in light mode
Rkoester47
left a comment
There was a problem hiding this comment.
I tested the functionality of the course notifications page on this branch, and everything seems to be working properly. I was able to click on individual notifications, mark individual notifications as seen, mark all as seen and also use the settings button. The home page notifications are also still working properly. The appearance of the notification page is clean and in line with the Home Page notifications now. Overall, this is a good change in terms of functionality and design as far as I can tell.
RyanStyron
left a comment
There was a problem hiding this comment.
I have thoroughly tested this refactor, and the functionality, sans the notification count updating, works as it did before.
|
@JManion32 |
|
Looks like accessibility & upducks & registration are failing. |
Why is this Change Important & Necessary?
The course notification page has a different UI from the home page:
Course Notifications
Home Page
What is the New Behavior?
The course notifications page has been updated to use the same Vue components as the home page:
By using the existing Vue component, we can delete most of
notifications.cssand all ofnotifications.twig.What steps should a reviewer take to reproduce or test the bug or new feature?
Ensure functionality of both course and home page notification panels remains intact and as expected.
Automated Testing & Documentation
The entire notification system does not yet have adequate e2e testing (see #11908). Perhaps the solution to that issue can include this new feature.
Other information
This shouldn't be a breaking change, however, it is altering the home page so if something goes wrong, it could prevent accessibility to the entire site.
When the seen count is updated, the sidebar count does not update dynamically:
I haven't been able to successfully implement this, but I do not think this PR should be held up by it. I will continue to look into it and either address here, or in a follow up PR.
sidebar.spec.jsverifies that the CSS content of each page is consistent. (contentexists undermain). Each Vue migration is breaking this test since the Vue component sits on top ofmain, rather than nested inside of it. The current trend is to add exceptions to the test, but we will need a more steadfast solution soon.The failing Ansible test seems to be unrelated to this PR, though this is the only one it is appearing on currently.