feat: Add HeaderNotificationsSlot to enable notifications tray default ON#663
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #663 +/- ##
==========================================
+ Coverage 72.22% 72.97% +0.75%
==========================================
Files 56 60 +4
Lines 504 518 +14
Branches 108 108
==========================================
+ Hits 364 378 +14
Misses 137 137
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'd like to break down the problems listed with the current approach a bit:
I think this part should be a separate PR. I'm not sure if adding a bell to the already very limited space in the mobile header makes sense. It'd be good to get some design/product opinions on this feature.
It is my understanding that this is the main motivation for this change. Overall I agree with this PR's approach as a solution to this.
I understand why this is frustrating, but the overall vision for slots is that they refer to places things can be put, not the default content in those places. I also know that isn't currently the case for quite a few existing slots (including One of the reasons for this is that "where it goes" gives site operators more granular control over how each page/part of a page looks, instead of having one slot change how multiple pages/parts of pages look. With that in mind, I think there's a way to get the best of both worlds here:
const DesktopSecondaryMenuSlot = ({
menu,
}) => (
<PluginSlot
id="org.openedx.frontend.layout.header_desktop_secondary_menu.v1"
idAliases={['desktop_secondary_menu_slot']}
slotOptions={{
mergeProps: true,
}}
>
+ <HeaderNotificationsSlot />
<DesktopHeaderMainOrSecondaryMenu menu={menu} />
</PluginSlot>
);
Hopefully these thoughts/suggestions make sense! Please let me know if you have any questions! |
@brian-smith-tcril Thank you for your detailed review and feedback. I have made the suggested changes. Please review it and let me know. |
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Thank you so much for addressing my previous comments! I'm very happy with this new structure!
I left quite a few comments, but they're pretty much all just README suggestions.
|
@brian-smith-tcril, I have incorporated all suggestions and included a screenshot. Please conduct a final review. Thank you. |
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Thanks for addressing my previous review comments! It looks like you added screenshots but didn't update the READMEs to include them. Once that's sorted out this should be good to go!
|
Thanks again for all the work on this! I noticed a couple things with the most recent changes. In bad7e4c you split the following slots into
I also feel like the changes to |
88a1940 to
99a6366
Compare
brian-smith-tcril
left a comment
There was a problem hiding this comment.
I gave this another pass. Thanks again for your patience throughout this review process. I know I've been focusing heavily on the READMEs for these slots. The reason for that focus is that site operators rely on these examples when using these slots, so having clear and accurate documentation is key.
brian-smith-tcril
left a comment
There was a problem hiding this comment.
This looks great! It's like 99.9% ready to land. There's one last tiny docs thing to clean up but once that's sorted this should be good to go!
Thanks again for working through the review process with me. I'm very happy with how this is coming together.
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Thank you so much for working with me throughout the review process! This looks great!




Ticket
Summery
Adds a new HeaderNotificationsSlot that renders NotificationsTray (from @edx/frontend-plugin-notifications) by default across Desktop, Learning, and Studio headers. This makes notifications available out-of-the-box for every Open edX instance, while allowing operators to disable or customize them via plugin slot configuration or the existing backend waffle flag.
Motivation
Currently, notifications in the header are enabled through tutor-contrib-platform-notifications, which injects NotificationsTray into 3 separate plugin slots:
Slots
org.openedx.frontend.layout.header_desktop_secondary_menu.v1 (Desktop Header)
org.openedx.frontend.layout.studio_header_search_button_slot.v1 (Studio Header)
org.openedx.frontend.layout.header_learning_help.v1 (Learning Header)
Problems with already implemented tutor-contrib-platform-notifications approach:
Implementation
New slot
v1/v2 split on three parent slots
Rather than embedding HeaderNotificationsSlot directly as a second child of an existing slot (which would break operators using the widget.content.* Modify pattern. React.isValidElement only works for single children), each affected slot is split into a v1/v2 pair.
Slot hierarchy
v2 is the default export consumed by header components.
How Operators Can Disable Notifications
Option 1: Plugin slot config (env.config.jsx)
Option 2:
Backend waffle flag - NotificationsTray is self-gating. It returns empty when the backend waffle flag is True. DISABLE_NOTIFICATIONS: True. Backend openedx/openedx-platform#38073
Backward Compatibility