-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat: Notification Plugin Slots #1368
Changes from 4 commits
377da57
d3ef306
1b5337a
1e29715
0989559
144a80a
af3eaf6
71a23d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import React, { useContext, useEffect, useMemo } from 'react'; | ||
|
||
import { sendTrackEvent } from '@edx/frontend-platform/analytics'; | ||
import { PluginSlot } from '@openedx/frontend-plugin-framework'; | ||
import { useModel } from '../../../../../../generic/model-store'; | ||
import UpgradeNotification from '../../../../../../generic/upgrade-notification/UpgradeNotification'; | ||
import { WIDGETS } from '../../../../../../constants'; | ||
|
@@ -66,24 +67,32 @@ const NotificationsWidget = () => { | |
|
||
if (hideNotificationbar || !isNotificationbarAvailable) { return null; } | ||
|
||
const upgradeNotificationProps = { | ||
offer, | ||
verifiedMode, | ||
accessExpiration, | ||
contentTypeGatingEnabled, | ||
marketingUrl, | ||
upsellPageName: 'in_course', | ||
userTimezone, | ||
timeOffsetMillis, | ||
courseId, | ||
org, | ||
upgradeNotificationCurrentState, | ||
setupgradeNotificationCurrentState: setUpgradeNotificationCurrentState, // TODO: Check typo in component? | ||
shouldDisplayBorder: false, | ||
toggleSidebar: () => toggleSidebar(currentSidebar, WIDGETS.NOTIFICATIONS), | ||
}; | ||
|
||
return ( | ||
<div className="border border-light-400 rounded-sm" data-testid="notification-widget"> | ||
<UpgradeNotification | ||
offer={offer} | ||
verifiedMode={verifiedMode} | ||
accessExpiration={accessExpiration} | ||
contentTypeGatingEnabled={contentTypeGatingEnabled} | ||
marketingUrl={marketingUrl} | ||
upsellPageName="in_course" | ||
userTimezone={userTimezone} | ||
shouldDisplayBorder={false} | ||
timeOffsetMillis={timeOffsetMillis} | ||
courseId={courseId} | ||
org={org} | ||
upgradeNotificationCurrentState={upgradeNotificationCurrentState} | ||
setupgradeNotificationCurrentState={setUpgradeNotificationCurrentState} | ||
toggleSidebar={() => toggleSidebar(currentSidebar, WIDGETS.NOTIFICATIONS)} | ||
/> | ||
<PluginSlot | ||
id="notification_widget" | ||
pluginProps={upgradeNotificationProps} | ||
testId="notification-widget-slot" | ||
> | ||
<UpgradeNotification {...upgradeNotificationProps} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same applies here as in the previous comment. |
||
</PluginSlot> | ||
</div> | ||
); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,6 +2,7 @@ import React, { Suspense } from 'react'; | |||||
import PropTypes from 'prop-types'; | ||||||
|
||||||
import { useIntl } from '@edx/frontend-platform/i18n'; | ||||||
import { PluginSlot } from '@openedx/frontend-plugin-framework'; | ||||||
|
||||||
import { useModel } from '@src/generic/model-store'; | ||||||
import PageLoading from '@src/generic/PageLoading'; | ||||||
|
@@ -24,19 +25,24 @@ const UnitSuspense = ({ | |||||
meta.contentTypeGatingEnabled && unit.containsContentTypeGatedContent | ||||||
); | ||||||
|
||||||
const suspenseComponent = (message, Component) => ( | ||||||
<Suspense fallback={<PageLoading srMessage={formatMessage(message)} />}> | ||||||
<Component courseId={courseId} /> | ||||||
</Suspense> | ||||||
); | ||||||
|
||||||
return ( | ||||||
<> | ||||||
{shouldDisplayContentGating && ( | ||||||
suspenseComponent(messages.loadingLockedContent, LockPaywall) | ||||||
<Suspense fallback={<PageLoading srMessage={formatMessage(messages.loadingLockedContent)} />}> | ||||||
<PluginSlot | ||||||
id="fbe_message_plugin" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following the new naming convention as outlined above:
Suggested change
Also, what does "fbe" stand for? Might be worth making the slot ID a little longer if it means it's easier to deduce what it's for just from the name. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's short for feature based enrollment, I went back and named this 'gated content' since that's a bit more general purpose anyway. |
||||||
pluginProps={{ | ||||||
courseId, | ||||||
}} | ||||||
> | ||||||
<LockPaywall courseId={courseId} /> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: "does it make sense for LockPaywall to remain on by default?" This is kinda the same as my comments above on 'DEPR'. I think my answer is initially yes we keep the default unchanged to limit risk. If this new plugin is deployed and we're confident there's no issues with the new experience we can remove the old default. This is a very sensitive component to alter. I'd like to keep 'turn off the plugin' as a switch we can pull rather than a large revert if there are problems. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have an idea of what the timeframe would be to gain the confidence necessary to do the removal? We need not remove the component itself from the codebase, but it would be best if we could at least remove it as default content inside the PluginSlot by Redwood. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we aren't actually removing the existing component we can import it inside the plugin as a second step. I'd say we can follow up with that part very quickly, maybe In a week or so. edit: a week after we turn this on, not a week after this is merged. |
||||||
</PluginSlot> | ||||||
</Suspense> | ||||||
)} | ||||||
{shouldDisplayHonorCode && ( | ||||||
suspenseComponent(messages.loadingHonorCode, HonorCode) | ||||||
<Suspense fallback={<PageLoading srMessage={formatMessage(messages.loadingHonorCode)} />}> | ||||||
<HonorCode courseId={courseId} /> | ||||||
</Suspense> | ||||||
)} | ||||||
</> | ||||||
); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; | |
import classNames from 'classnames'; | ||
import React, { useContext, useEffect, useMemo } from 'react'; | ||
import { sendTrackEvent } from '@edx/frontend-platform/analytics'; | ||
import { PluginSlot } from '@openedx/frontend-plugin-framework'; | ||
import { useModel } from '../../../../../generic/model-store'; | ||
import UpgradeNotification from '../../../../../generic/upgrade-notification/UpgradeNotification'; | ||
|
||
|
@@ -65,6 +66,22 @@ const NotificationTray = ({ intl }) => { | |
sendTrackEvent('edx.ui.course.upgrade.old_sidebar.notifications', notificationTrayEventProperties); | ||
}, []); | ||
|
||
const upgradeNotificationProps = { | ||
offer, | ||
verifiedMode, | ||
accessExpiration, | ||
contentTypeGatingEnabled, | ||
marketingUrl, | ||
upsellPageName: 'in_course', | ||
userTimezone, | ||
shouldDisplayBorder: false, | ||
timeOffsetMillis, | ||
courseId, | ||
org, | ||
upgradeNotificationCurrentState, | ||
setupgradeNotificationCurrentState: setUpgradeNotificationCurrentState, // TODO: Check typo in component? | ||
}; | ||
|
||
return ( | ||
<SidebarBase | ||
title={intl.formatMessage(messages.notificationTitle)} | ||
|
@@ -75,21 +92,13 @@ const NotificationTray = ({ intl }) => { | |
> | ||
<div>{verifiedMode | ||
? ( | ||
<UpgradeNotification | ||
offer={offer} | ||
verifiedMode={verifiedMode} | ||
accessExpiration={accessExpiration} | ||
contentTypeGatingEnabled={contentTypeGatingEnabled} | ||
marketingUrl={marketingUrl} | ||
upsellPageName="in_course" | ||
userTimezone={userTimezone} | ||
shouldDisplayBorder={false} | ||
timeOffsetMillis={timeOffsetMillis} | ||
courseId={courseId} | ||
org={org} | ||
upgradeNotificationCurrentState={upgradeNotificationCurrentState} | ||
setupgradeNotificationCurrentState={setUpgradeNotificationCurrentState} | ||
/> | ||
<PluginSlot | ||
id="notification_tray" | ||
pluginProps={upgradeNotificationProps} | ||
testId="notification-tray-slot" | ||
> | ||
<UpgradeNotification {...upgradeNotificationProps} /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Plus, we should check with Product what to do if |
||
</PluginSlot> | ||
) : ( | ||
<p className="p-3 small">{intl.formatMessage(messages.noNotificationsMessage)}</p> | ||
)} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
|
||
const MockedPluginSlot = ({ children, testId }) => { | ||
if (!testId) { return children ?? 'PluginSlot'; } // Return its content if PluginSlot slot is wrapping any. | ||
|
||
return <div data-testid={testId}>{children}</div>; | ||
}; | ||
|
||
MockedPluginSlot.displayName = 'PluginSlot'; | ||
|
||
MockedPluginSlot.propTypes = { | ||
children: PropTypes.oneOfType([ | ||
PropTypes.arrayOf(PropTypes.node), | ||
PropTypes.node, | ||
]), | ||
testId: PropTypes.string, | ||
}; | ||
|
||
MockedPluginSlot.defaultProps = { | ||
children: undefined, | ||
testId: undefined, | ||
}; | ||
|
||
export default MockedPluginSlot; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import React from 'react'; | ||
import { render, screen } from '@testing-library/react'; | ||
import MockedPluginSlot from './MockedPluginSlot'; | ||
|
||
describe('MockedPluginSlot', () => { | ||
it('renders as plain "PluginSlot" text node if no clildren nor testId is', () => { | ||
render(<MockedPluginSlot />); | ||
|
||
const component = screen.getByText('PluginSlot'); | ||
expect(component).toBeInTheDocument(); | ||
}); | ||
|
||
it('renders as the slot children directly if there is content within and no testId', () => { | ||
render( | ||
<div role="article"> | ||
<MockedPluginSlot> | ||
<q role="note">How much wood could a woodchuck chuck if a woodchuck could chuck wood?</q> | ||
</MockedPluginSlot> | ||
</div>, | ||
); | ||
|
||
const component = screen.getByRole('article'); | ||
expect(component).toBeInTheDocument(); | ||
|
||
// Direct children | ||
const quote = component.querySelector(':scope > q'); | ||
expect(quote.getAttribute('role')).toBe('note'); | ||
}); | ||
|
||
it('renders a div when a testId is provided ', () => { | ||
render( | ||
<MockedPluginSlot testId="guybrush"> | ||
<q role="note">I am selling these fine leather jackets.</q> | ||
</MockedPluginSlot>, | ||
); | ||
|
||
const component = screen.getByTestId('guybrush'); | ||
expect(component).toBeInTheDocument(); | ||
|
||
const quote = component.querySelector('[role=note]'); | ||
expect(quote).toBeInTheDocument(); | ||
}); | ||
}); |
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.
A few things:
UpgradeNotification
component should not be present by defaultUpgradeNotification
, the component itself should be removed from the codebase (and made into a plugin that lives elsewhere)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'd agree the
UpgradeNotification
is likely not used by other sites but hard to know for sure. For points 1 and 2 would it make sense to separate the actual DEPR of this component from the additive change here? It seems to me we can separate these two initiatives.There's probably an opportunity to narrow down or fully remove props passed into this. I'll have us take a look into that. Removing anything that isn't direct context of the parent location (OutlineTab) would help us keep this more generalized for the future. But, I'd hesitate to over architect how this slot is works, such as requiring a backend package for props, if we don't have any other use cases yet. For this case, our plugin is just small UX changes that don't require a backend.
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.
On the DEPR front, there's some nuance here that we need to explore. From Open edX's point of view, the deprecation and removal process would ideally happen before the Redwood cutoff on May 9th. This is because 1) this code doesn't exist in Quince, and 2) if it becomes part of Redwood we'll have to support it for much longer, and I believe the consensus on the Axim engineering side is that we don't want the community to have to do that.
Also, to make sure we're on the same page, since this has been found not to be a part of the core Open edX product (as per the roadmap proposal: openedx/platform-roadmap#332 (comment)), it should leave no room for doubt whether it should stay in or not. It's just a matter of when.
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 concern here is that by creating a PluginSlot, we're actually creating an API entrypoint. We want to strive to make it both generic and stable. If the plugin slot only works with a very specific component, then it's not much better than just having the component be there behind a configuration variable. And if we don´t make it stable, it doesn't make it very useful for people that don´t want to fork the codebase to introduce something here.
Hence, the effort to generalize this as much as we can. It might not be entirely possible given the short time-frame, but I believe we should at least give it our best shot.
I think there might be an example of a callback doing something about generalization in the AI Translation plugin implementation. Let me see if I can find it.
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.
Thanks, I'm actually exploring the option that maybe we simply shouldn't be passing props at all to these slots. I suspect everything we're using can be gotten from redux on the plugin side. That seems like a much simpler and generic pattern to follow than trying to maintain an API that's likely to change despite our best efforts.
If that works is that a pattern we'd be okay with?
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.
Found it:
https://github.com/openedx/frontend-app-learning/blob/master/src/generic/plugin-store/hooks.js#L4
@leangseu-edx came up with a way for plugins to register callback functions that can act on a plugin-store redux slice.
After you've checked which props are actually necessary, and if any feature-specific ones remain, I'd be interested in investigating if we could generalize them with methods such as the above.
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 does not need a DEPR, it can just be removed. It's 2U specific and has not been released in an Open edX release so you can simply make the PR to remove it.
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.
Good to know we can remove this without DEPR. I'd still propose we make the additive change here that keeps the current learning MFE code as-is but pluggable and follow up with a separate PR that removes the upgrade notification and paywall components from this repo entirely. That is assuming the properties we're passing to the plugin are solved to be generic or removed entirely.
I don't think we could hit that Redwood cutoff if we must include the removal of the current components and any supporting code in this MFE. That work opens up some additional unsolved issues around where the translations go I'd rather not get into if we can help it.
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.
@arbrandes I've put together a quick POC of removing 99% of these parameters here: #1379.
That is just the sidebar since that's the most complex bit. If this approach generally looks good we can do the same for everything and update this PR.
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 going with
useModel()
is a good direction to take, for now. It doesn't mean we're guaranteeing that the data structures will remain the same, but it will be a demonstration that you can technically do it if you're fine with bearing the risk as a plugin author.In other words, it works for us if it works for you.