Adding new slots for Notification Tray integration #644

farhaanbukhsh · 2025-10-12T07:25:40Z

Addition of the Header Item slot, this slot is created with the intention to add Notitification Tray to the learner dashboard, studio, and learning MFE. This in turn helps to add the same slot to other MFEs as well since all of them use the same header.

JIRA tickets: None

Discussions: Link to any public dicussions about this PR or the design/architecture. Otherwise omit this.

Dependencies: None

Screenshots:

Included in the Readme.md

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: "None" if there's no rush, "ASAP" if it's critical, or provide a specific date if there is one.

Testing instructions:

Clone this repo and change to this branch. Run npm ci && npm run build
Let's checkout the learner dashboard to have the notification tray slot.
If you have a tutor devstack already running, stop the learner dashboard, tutor dev stop learner-dashboard
Make sure learner-dashboard is already mounted you can check tutor mount lists if is is not mounted you can add tutor mounts add /path_to/edx-devstack/frontend-app-learner-dashboard and build the image tutor images build openedx-dev
Now we need to copy the dist/ folder under frontend-component-header. cp -r /path_to/edx-devstack/frontend-component-header/dist /path_to/edx-devstack/frontend-app-learner-dashboard/node_modules/@edx/frontend-component-header. This copies the built codebase to the frontend-component-header
Add env.config.jsx in /path_to/edx-devstack/frontend-app-learner-dashboard with the content,

import { PLUGIN_OPERATIONS, DIRECT_PLUGIN } from '@openedx/frontend-plugin-framework';
import { NotificationsTray, useAppNotifications, Notifications } from '@edx/frontend-plugin-notifications';
// Load environment variables from .env file
const config = {
  pluginSlots: {
    "org.openedx.frontend.layout.desktop_header_item_slot.v1": {
      keepDefault: false,
      plugins: [
        {
          op: PLUGIN_OPERATIONS.Insert,
          widget: {
            id: 'custom_notification_tray',
            type: DIRECT_PLUGIN,
            RenderWidget: () => (
              <NotificationsTray />
            ),
          },
        },
      ],
    },
  },
};

export default config;

We need to install the notification plugin in learner dashboard i.e /path_to/edx-devstack/frontend-app-learner-dashboard npm i @edx/frontend-plugin-notifications@^2.0.3
Now we can run npm run dev, after this if we see learner dashboard we should see notification icon button.
Similar configs can be created for other MFEs to test them locally.

Private Refs:
https://tasks.opencraft.com/browse/BB-10062
https://tasks.opencraft.com/browse/BB-10060

openedx-webhooks · 2025-10-12T07:25:45Z

Thanks for the pull request, @farhaanbukhsh!

This repository is currently maintained by @openedx/committers-frontend.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

codecov · 2025-10-12T11:01:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.22%. Comparing base (9e850c3) to head (b4dd7e0).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #644      +/-   ##
==========================================
+ Coverage   72.11%   72.22%   +0.11%     
==========================================
  Files          55       56       +1     
  Lines         502      504       +2     
  Branches      108      108              
==========================================
+ Hits          362      364       +2     
  Misses        137      137              
  Partials        3        3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tecoholic

@farhaanbukhsh Great work on handling the multiple scenarios here. Thanks for the detailed testing instructions. I followed them and they mostly worked except for the <NotificationsTray /> component never showing up. I change to simple span like the ones from the README and that worked just fine.

I have left inline comments. I want to note a couple of things that I noticed here:

Missing `priority` in the `widget` definitions

When I inpect the component tree in React Dev tools, I have this error on the slot, for all the slots

the insert operation config is invalid for widget id: mobile_notification_tray undefined

this error goes away only when the priority key is added to the widget object. So, all the code examples in the README file will need to be updated to include this property.

JSX `key` spread warning

There is also another error in the component tree with an ErrorBoundary

Warning: A props object containing a "key" prop is being spread into JSX: let props = {key: someKey, id: ...}; <RenderWidget {...props} /> React keys must be passed directly to JSX without using spread: let props = {id: ...}; <RenderWidget key={someKey} {...props} /> at PluginContainerDirect (webpack-internal:///./node_modules/@openedx/frontend-plugin-framework/dist/plugins/PluginContainerDirect.js:25:7) at ErrorBoundary (webpack-internal:///./node_modules/@edx/frontend-platform/react/ErrorBoundary.js:104:5) at PluginContainer (webpack-internal:///./node_modules/@openedx/frontend-plugin-framework/dist/plugins/PluginContainer.js:27:7) at BasePluginSlot (webpack-internal:///./node_modules/@openedx/frontend-plugin-framework/dist/plugins/PluginSlot.js:31:7) at MobileHeaderItemSlot at button at MenuTrigger (webpack-internal:///./node_modules/@edx/frontend-component-header/dist/Menu/Menu.js:146:18) at nav at Menu (webpack-internal:///./node_modules/@edx/frontend-component-header/dist/Menu/Menu.js:196:5) at div at header at MobileHeader (webpack-internal:///./node_modules/@edx/frontend-component-header/dist/mobile-header/MobileHeader.js:82:23) at BasePluginSlot (webpack-internal:///./node_modules/@openedx/frontend-plugin-framework/dist/plugins/PluginSlot.js:31:7) at MobileHeaderSlot (webpack-internal:///./node_modules/@edx/frontend-component-header/dist/plugin-slots/MobileHeaderSlot/index.js:13:20) at MediaQuery (webpack-internal:///./node_modules/react-responsive/dist/react-responsive.js:868:33) at Header (webpack-internal:///./node_modules/@edx/frontend-component-header/dist/Header.js:77:28) at LearnerDashboardHeader (webpack-internal:///./src/containers/LearnerDashboardHeader/index.jsx:36:52) at AppWrapper (webpack-internal:///./src/containers/AppWrapper/index.jsx:15:5) at div at App (webpack-internal:///./src/App.jsx:64:52) at PageWrap (webpack-internal:///./node_modules/@edx/frontend-platform/react/PageWrap.js:25:23) at RenderedRoute (webpack-internal:///./node_modules/react-router/dist/index.js:579:5) at Routes (webpack-internal:///./node_modules/react-router/dist/index.js:1313:5) at div at NoticesWrapper (webpack-internal:///./src/components/NoticesWrapper/index.jsx:30:5) at div at Router (webpack-internal:///./node_modules/react-router/dist/index.js:1247:15) at BrowserRouter (webpack-internal:///./node_modules/react-router-dom/dist/index.js:704:5) at div at Provider (webpack-internal:///./node_modules/react-redux/es/components/Provider.js:19:20) at OptionalReduxProvider (webpack-internal:///./node_modules/@edx/frontend-platform/react/OptionalReduxProvider.js:19:25) at ErrorBoundary (webpack-internal:///./node_modules/@edx/frontend-platform/react/ErrorBoundary.js:104:5) at IntlProvider (webpack-internal:///./node_modules/react-intl/lib/src/components/provider.js:42:47) at AppProvider (webpack-internal:///./node_modules/@edx/frontend-platform/react/AppProvider.js:117:25)

I am NOT certain about the reason for this error, but I suspect it's probably connected to not defining any properties in the slots. Thus, making the plugin-slots library's implementation do something that React doesn't like.

src/mobile-header/MobileHeader.jsx

src/plugin-slots/DesktopHeaderItemSlot/images/learner_dashboard_notification.png

src/plugin-slots/LearningHeaderItemSlot/images/learner_header_notification.png

src/plugin-slots/DesktopHeaderItemSlot/README.md

src/desktop-header/DesktopHeader.jsx

src/__snapshots__/Header.test.jsx.snap

src/Header.messages.jsx

farhaanbukhsh · 2025-10-13T10:40:31Z

@farhaanbukhsh Great work on handling the multiple scenarios here. Thanks for the detailed testing instructions. I followed them and they mostly worked except for the <NotificationsTray /> component never showing up. I change to simple span like the ones from the README and that worked just fine.

npm i @edx/frontend-plugin-notifications@^2.0.3 should solve it possibly the npm directly pulled from github and didn't run a build on it.

I have left inline comments. I want to note a couple of things that I noticed here:

Missing priority in the widget definitions

When I inpect the component tree in React Dev tools, I have this error on the slot, for all the slots
the insert operation config is invalid for widget id: mobile_notification_tray undefined
this error goes away only when the priority key is added to the widget object. So, all the code examples in the README file will need to be updated to include this property.

Great I will add the priority to the readme.md @tecoholic

JSX `key` spread warning

There is also another error in the component tree with an ErrorBoundary

Warning: A props object containing a "key" prop is being spread into JSX: let props = {key: someKey, id: ...}; <RenderWidget {...props} /> React keys must be passed directly to JSX without using spread: let props = {id: ...}; <RenderWidget key={someKey} {...props} /> at PluginContainerDirect (webpack-internal:///./node_modules/@openedx/frontend-plugin-framework/dist/plugins/PluginContainerDirect.js:25:7) at ErrorBoundary (webpack-internal:///./node_modules/@edx/frontend-platform/react/ErrorBoundary.js:104:5) at PluginContainer (webpack-internal:///./node_modules/@openedx/frontend-plugin-framework/dist/plugins/PluginContainer.js:27:7) at BasePluginSlot (webpack-internal:///./node_modules/@openedx/frontend-plugin-framework/dist/plugins/PluginSlot.js:31:7) at MobileHeaderItemSlot at button at MenuTrigger (webpack-internal:///./node_modules/@edx/frontend-component-header/dist/Menu/Menu.js:146:18) at nav at Menu (webpack-internal:///./node_modules/@edx/frontend-component-header/dist/Menu/Menu.js:196:5) at div at header at MobileHeader (webpack-internal:///./node_modules/@edx/frontend-component-header/dist/mobile-header/MobileHeader.js:82:23) at BasePluginSlot (webpack-internal:///./node_modules/@openedx/frontend-plugin-framework/dist/plugins/PluginSlot.js:31:7) at MobileHeaderSlot (webpack-internal:///./node_modules/@edx/frontend-component-header/dist/plugin-slots/MobileHeaderSlot/index.js:13:20) at MediaQuery (webpack-internal:///./node_modules/react-responsive/dist/react-responsive.js:868:33) at Header (webpack-internal:///./node_modules/@edx/frontend-component-header/dist/Header.js:77:28) at LearnerDashboardHeader (webpack-internal:///./src/containers/LearnerDashboardHeader/index.jsx:36:52) at AppWrapper (webpack-internal:///./src/containers/AppWrapper/index.jsx:15:5) at div at App (webpack-internal:///./src/App.jsx:64:52) at PageWrap (webpack-internal:///./node_modules/@edx/frontend-platform/react/PageWrap.js:25:23) at RenderedRoute (webpack-internal:///./node_modules/react-router/dist/index.js:579:5) at Routes (webpack-internal:///./node_modules/react-router/dist/index.js:1313:5) at div at NoticesWrapper (webpack-internal:///./src/components/NoticesWrapper/index.jsx:30:5) at div at Router (webpack-internal:///./node_modules/react-router/dist/index.js:1247:15) at BrowserRouter (webpack-internal:///./node_modules/react-router-dom/dist/index.js:704:5) at div at Provider (webpack-internal:///./node_modules/react-redux/es/components/Provider.js:19:20) at OptionalReduxProvider (webpack-internal:///./node_modules/@edx/frontend-platform/react/OptionalReduxProvider.js:19:25) at ErrorBoundary (webpack-internal:///./node_modules/@edx/frontend-platform/react/ErrorBoundary.js:104:5) at IntlProvider (webpack-internal:///./node_modules/react-intl/lib/src/components/provider.js:42:47) at AppProvider (webpack-internal:///./node_modules/@edx/frontend-platform/react/AppProvider.js:117:25)

I am NOT certain about the reason for this error, but I suspect it's probably connected to not defining any properties in the slots. Thus, making the plugin-slots library's implementation do something that React doesn't like.

I don't see this on the sandbox that we have setup so I am assuming it wouldn't be a big thing and it should be handled on the building step and is a problem for dev build.

farhaanbukhsh · 2025-10-14T09:27:53Z

@tecoholic @arbrandes @xitij2000 The sandbox https://axim-notification.do.opencraft.hosting/ has these changes so that you can test them. The configuration is passed through openedx/tutor-contrib-platform-notifications#5

tecoholic

@farhaanbukhsh 👍 Thank you for addressing all the comments. LGTM.

I tested this: Tested all the slots and verified that they are placed in the right places and function as intended.
I read through the code
I checked for accessibility issues
Includes documentation

src/studio-header/HeaderBody.tsx

brian-smith-tcril

I don't think we should add the slots that are currently in this PR.

I left some comments with more details, but overall:

I don't see how DesktopHeaderItemSlot and LearningHeaderItemSlot are adding anything that can't be accomplished with existing slots
MobileHeaderItemSlot and StudioHeaderItemSlot are adding new functionality, but in "serves one specific use case" ways. I left suggestions about ways to add the functionality in more versatile ways.

src/plugin-slots/DesktopHeaderItemSlot/README.md

src/plugin-slots/LearningHeaderItemSlot/README.md

src/mobile-header/MobileHeader.jsx

src/plugin-slots/StudioHeaderItemSlot/README.md

brian-smith-tcril

Architecturally this is great! Very glad to see that you were able to utilize existing slots, and I'm glad to see wrapping the search button works for your purposes as well!

The only changes I'd like to see here are naming related. I'm open to other thoughts/suggestions, but considering this is a slot that wraps the search button, I think it makes sense to call it the search button slot. While not explicitly mentioned in the plugin slot naming and life cycle ADR, the general pattern has been to name slots that are wrapping something based on what they are wrapping.

So this would mean:

StudioHeaderItemSlot -> StudioHeaderSearchButtonSlot
org.openedx.frontend.layout.studio_header_item_slot.v1 -> org.openedx.frontend.layout.studio_header_search_button_slot.v1

Thank you so much for working through the review process with me on this PR!

farhaanbukhsh · 2025-10-27T03:15:37Z

@brian-smith-tcril thanks a lot for a such a quick review, I have made the changes you have asked for, please have a lot when you get a chance. Thanks again for such great reviews.

brian-smith-tcril

LGTM!

One tiny suggestion in the readme description but I don't want to block this PR on that.

Thank you so much for putting all of this together!

brian-smith-tcril · 2025-10-27T06:51:21Z

src/plugin-slots/StudioHeaderSearchButtonSlot/README.md

+
+## Description
+
+This slot is used to replace/modify/hide the action items in the studio header.


Suggested change

This slot is used to replace/modify/hide the action items in the studio header.

This slot is used to replace/modify/hide the search button in the studio header.

@brian-smith-tcril thank you for the review and done! :)

Please merge it if you have the access and if everything looks oka @brian-smith-tcril , thanks a lot again for the work and effort.

Signed-off-by: Farhaan Bukhsh <[email protected]>

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Oct 12, 2025

openedx-webhooks added this to Contributions Oct 12, 2025

github-project-automation bot moved this to Needs Triage in Contributions Oct 12, 2025

farhaanbukhsh force-pushed the farhaan/refactoring-slots branch from f255ff6 to e60fac3 Compare October 12, 2025 11:13

farhaanbukhsh changed the title ~~Farhaan/refactoring slots~~ Adding new slots for Notification Tray integration Oct 12, 2025

This was referenced Oct 12, 2025

feat: Adds notification tray slot #638

Closed

Adding and upstreaming NotificationSlot openedx/frontend-plugin-notifications#50

Closed

farhaanbukhsh requested review from arbrandes and xitij2000 October 12, 2025 15:41

tecoholic suggested changes Oct 13, 2025

View reviewed changes

farhaanbukhsh mentioned this pull request Oct 14, 2025

feat: Initial implementation openedx/tutor-contrib-platform-notifications#3

Closed

farhaanbukhsh force-pushed the farhaan/refactoring-slots branch from 31b5286 to ffa337b Compare October 14, 2025 12:59

farhaanbukhsh requested a review from tecoholic October 15, 2025 11:08

tecoholic approved these changes Oct 15, 2025

View reviewed changes

src/studio-header/HeaderBody.tsx Outdated Show resolved Hide resolved

xitij2000 approved these changes Oct 15, 2025

View reviewed changes

mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Oct 15, 2025

brian-smith-tcril reviewed Oct 21, 2025

View reviewed changes

farhaanbukhsh force-pushed the farhaan/refactoring-slots branch 2 times, most recently from 4642e0b to 47cfc6f Compare October 25, 2025 15:25

farhaanbukhsh requested a review from brian-smith-tcril October 25, 2025 15:29

brian-smith-tcril requested changes Oct 27, 2025

View reviewed changes

farhaanbukhsh force-pushed the farhaan/refactoring-slots branch 4 times, most recently from 1edfa42 to bfb921e Compare October 27, 2025 03:14

farhaanbukhsh requested a review from brian-smith-tcril October 27, 2025 03:17

brian-smith-tcril approved these changes Oct 27, 2025

View reviewed changes

feat: Moves search button into a slot

b4dd7e0

Signed-off-by: Farhaan Bukhsh <[email protected]>

farhaanbukhsh force-pushed the farhaan/refactoring-slots branch from bfb921e to b4dd7e0 Compare October 27, 2025 07:14

farhaanbukhsh requested a review from brian-smith-tcril October 27, 2025 07:26

brian-smith-tcril merged commit c861651 into openedx:master Oct 27, 2025
6 checks passed

github-project-automation bot moved this from In Eng Review to Done in Contributions Oct 27, 2025

This was referenced Oct 27, 2025

feat: Adds search slot integration for notification tray openedx/tutor-contrib-platform-notifications#8

Merged

chore: Updates frontend-component-header to include new slots openedx/frontend-app-authoring#2576

Merged


		## Description

		This slot is used to replace/modify/hide the action items in the studio header.

Adding new slots for Notification Tray integration #644

Adding new slots for Notification Tray integration #644

Uh oh!

Conversation

farhaanbukhsh commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Oct 12, 2025

Uh oh!

codecov bot commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tecoholic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Missing priority in the widget definitions

JSX key spread warning

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

farhaanbukhsh commented Oct 13, 2025

Missing priority in the widget definitions

JSX key spread warning

Uh oh!

farhaanbukhsh commented Oct 14, 2025

Uh oh!

tecoholic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Uh oh!

farhaanbukhsh commented Oct 27, 2025

Uh oh!

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Uh oh!

brian-smith-tcril Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

farhaanbukhsh Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

farhaanbukhsh Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

farhaanbukhsh commented Oct 12, 2025 •

edited

Loading

codecov bot commented Oct 12, 2025 •

edited

Loading

tecoholic left a comment •

edited

Loading

Missing `priority` in the `widget` definitions

JSX `key` spread warning

Missing `priority` in the `widget` definitions

JSX `key` spread warning