-
Notifications
You must be signed in to change notification settings - Fork 211
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 7 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 | ||||
---|---|---|---|---|---|---|
|
@@ -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 |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
|
||
const MockedPluginSlot = ({ children, id }) => ( | ||
<div data-testid={id}> | ||
PluginSlot_{id} | ||
{ children && <div>{children}</div> } | ||
</div> | ||
); | ||
|
||
MockedPluginSlot.displayName = 'PluginSlot'; | ||
|
||
MockedPluginSlot.propTypes = { | ||
children: PropTypes.oneOfType([ | ||
PropTypes.arrayOf(PropTypes.node), | ||
PropTypes.node, | ||
]), | ||
id: PropTypes.string, | ||
}; | ||
|
||
MockedPluginSlot.defaultProps = { | ||
children: undefined, | ||
id: undefined, | ||
}; | ||
|
||
export default MockedPluginSlot; |
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.
We're trying to standardize how slots are exposed (details here: openedx/wg-frontend#178). I won't ask you to tackle all that here (we'll probably do it ourselves in this other PR), but in order to avoid a breaking change to the slot ID, do you mind changing it as suggested below?
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.
ahh sounds good I can rename both of these