-
Notifications
You must be signed in to change notification settings - Fork 163
Adding new slots for Notification Tray integration #644
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
base: master
Are you sure you want to change the base?
Adding new slots for Notification Tray integration #644
Conversation
Thanks for the pull request, @farhaanbukhsh! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #644 +/- ##
==========================================
- Coverage 73.01% 72.40% -0.61%
==========================================
Files 55 59 +4
Lines 504 511 +7
Branches 100 108 +8
==========================================
+ Hits 368 370 +2
- Misses 133 138 +5
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Farhaan Bukhsh <[email protected]>
Signed-off-by: Farhaan Bukhsh <[email protected]>
Signed-off-by: Farhaan Bukhsh <[email protected]>
f255ff6
to
e60fac3
Compare
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.
@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
Outdated
<MenuTrigger | ||
tag="button" | ||
className="icon-button" | ||
aria-label={intl.formatMessage(messages['header.label.mobile.item.slot'])} | ||
title={intl.formatMessage(messages['header.label.mobile.item.slot'])} | ||
> | ||
{renderMobileHeaderItemSlot()} | ||
</MenuTrigger> |
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.
<MenuTrigger | |
tag="button" | |
className="icon-button" | |
aria-label={intl.formatMessage(messages['header.label.mobile.item.slot'])} | |
title={intl.formatMessage(messages['header.label.mobile.item.slot'])} | |
> | |
{renderMobileHeaderItemSlot()} | |
</MenuTrigger> | |
<MobileHeaderItemSlot /> |
Otherwise the MenuTrigger
would act on the both the UserIcon and the slot. For example, notice the menu opening for the "MOB" button.

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.
This image doesn't correspond to the code in the example. So, it might be a bit confusing for the reader to notice the change.
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.
Same feeback as the previous screenshot. We either need to show the same content, so it matches or mark the slot in the picture so, the reader knows where to expect the content.
The slot is kept empty so that it can only be enabled when it is required else | ||
nothing is displayed. It is used to add item in the frontend-app-learner-dashboard | ||
header. |
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.
The slot is named as DesktopHeaderItemSlot
, but here it is mentioned it only adds an item in the learner-dashboard
MFE. Shouldn't it them be LearnerDashboardHeaderItemSlot
following the convention of StudioHeaderItemSlot
LearningHeaderItemSlot
..etc.,?
Or does this slot apply to all the other MFEs that doesn't have specific headers like Studio & Learning MFE?
Either way, a description of what this slot is, with specifics on where it does & doesn't apply would be better here.
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.
it's mostly the slots which doesn't have any specific header @tecoholic I have changed it to match other Readme under same heading.
src/desktop-header/DesktopHeader.jsx
Outdated
{loggedIn | ||
? ( | ||
<> | ||
{renderDesktopHeaderItemSlot()} |
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.
Any specific reason not to simply do this?
{renderDesktopHeaderItemSlot()} | |
<DesktopHeaderItemSlot /> |
onMouseEnter={[Function]} | ||
onMouseLeave={[Function]} | ||
> | ||
<button |
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.
I think this shouldn't be needed here and it probably an artifact due to the MenuTrigger
wrapper in the MobileHeader.jsx
below.
src/Header.messages.jsx
Outdated
'header.label.mobile.item.slot': { | ||
id: 'header.label.mobile.item.slot', | ||
defaultMessage: 'Mobile Item Slot', | ||
description: 'The aria label for plugin item slot for mobile', | ||
}, |
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.
If this is specific to the Header tests, then this shouldn't be needed when the MenuTrigger
is removed.
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.
Great I will add the priority to the readme.md @tecoholic
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. |
@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-notifications#5 |
Signed-off-by: Farhaan Bukhsh <[email protected]>
Signed-off-by: Farhaan Bukhsh <[email protected]>
Signed-off-by: Farhaan Bukhsh <[email protected]>
31b5286
to
ffa337b
Compare
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.
@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
Outdated
<Container | ||
size="xl" | ||
className={classNames('px-2.5', containerClassName)} | ||
className={classNames('px-0', containerClassName)} |
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.
Curious question: how does this padding affects the slot?
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.
@tecoholic it gives breathing space to the content of the header else the header was looking very squishy
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.
I'm unsure of this change. Perhaps it should be in a separate PR, it has nothing to do with the slots.
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.
Okay let me undo this :)
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.
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
andLearningHeaderItemSlot
are adding anything that can't be accomplished with existing slots MobileHeaderItemSlot
andStudioHeaderItemSlot
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.
@@ -0,0 +1,50 @@ | |||
# Desktop Header Item Slot |
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.
My main question about this slot is just, "Is it needed?"
It's not clear to me what functionality is it providing that cannot be accomplished by following the "Add Custom Components before and after Menu" example for org.openedx.frontend.layout.header_desktop_secondary_menu.v1
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.
I think you are right here, the use case of adding before and after makes this simpler. Thank you for introducing this to me :)
@@ -0,0 +1,51 @@ | |||
# Learning Header Item Slot |
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.
Similar to my question about org.openedx.frontend.layout.desktop_header_item_slot.v1
- it's not clear to me what functionality this is providing that isn't available by using org.openedx.frontend.layout.header_learning_help.v1
.
I realize the only example we have for the Learning Help Slot is fully replacing it, but we could definitely add a "keepDefault
/insert before" example.
{userMenu.length > 0 || loggedOutItems.length > 0 ? ( | ||
<div className="w-100 d-flex justify-content-end align-items-center"> | ||
<Menu tag="nav" aria-label={intl.formatMessage(messages['header.label.secondary.nav'])} className="position-static"> | ||
<MobileHeaderItemSlot /> |
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.
It seems odd to have this inside the Menu component. Does it not work when moved up one level?
I also think it might make more sense/provide more flexibility if we added a slot that wraps the entirety of the mobile user and logged out items menus.
We currently have org.openedx.frontend.layout.header_mobile_user_menu.v1
which wraps the user menu content, org.openedx.frontend.layout.header_mobile_logged_out_items.v1
which wraps the logged out items menu content, and org.openedx.frontend.layout.header_mobile_user_menu_trigger.v1
which wraps the content of the MenuTrigger
button, but nothing that wraps it all at the top level.
If we had a slot that wrapped the entire thing, then we could use the "keepDefault
/insert before" pattern for this.
@@ -0,0 +1,51 @@ | |||
# Studio Header Item Slot |
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.
Considering the use case for this slot, I think it would make sense to instead create a slot that wraps the search button.
That would:
- Allow plugin authors following the
keepDefault
pattern to either insert before, after, or both instead of only being able to insert before the search button - Allow plugin authors to completely replace the search button if desired
- Effectively communicate what the slot is and where it lives.
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:
npm ci && npm run build
tutor dev stop learner-dashboard
tutor mount lists
if is is not mounted you can addtutor mounts add /path_to/edx-devstack/frontend-app-learner-dashboard
and build the imagetutor images build openedx-dev
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-headernpm i @edx/frontend-plugin-notifications@^2.0.3
npm run dev
, after this if we see learner dashboard we should see notification icon button.