feat: removes sidebar upgrade notification component #1410
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1410 +/- ##
==========================================
+ Coverage 88.87% 89.02% +0.15%
==========================================
Files 307 306 -1
Lines 5285 5168 -117
Branches 1344 1280 -64
==========================================
- Hits 4697 4601 -96
+ Misses 572 551 -21
Partials 16 16 ☔ View full report in Codecov by Sentry. |
| @@ -34,9 +33,7 @@ const UnitSuspense = ({ | |||
| pluginProps={{ | |||
| courseId, | |||
| }} | |||
| > | |||
| <LockPaywall courseId={courseId} /> | |||
There was a problem hiding this comment.
I had this removed originally but after thinking it through my instinct is we actually don't want to touch this. I'll leave it here for now to discuss.
Is content gating edx.org specific or a core feature of Open edX that should have an associated paywall/view in this MFE?
There was a problem hiding this comment.
My instinct is that this should remain as it feels like a core feature of Open edX, but I'm guessing you're leaving here to confirm with Axim?
| jest.mock('@edx/frontend-platform/analytics'); | ||
|
|
||
| /* eslint-disable react/prop-types */ | ||
| jest.mock('@openedx/frontend-plugin-framework', () => ({ |
There was a problem hiding this comment.
this is the only addition here since there are tests that rely on a close button within the notification that toggle the sidebar. We need a 'mock' notification now since there is no default
|
FYI @arbrandes |
| } = useModel('courseHomeMeta', courseId); | ||
|
|
||
| const { | ||
| accessExpiration, |
There was a problem hiding this comment.
The props removed here are just the ones that are no longer being used by the UpgradeNotification, correct?
| @@ -198,21 +187,7 @@ const OutlineTab = ({ intl }) => { | |||
| <PluginSlot | |||
| id="outline_tab_notifications_slot" | |||
| pluginProps={{ courseId }} | |||
| > | |||
| @@ -81,24 +74,7 @@ const NotificationsWidget = () => { | |||
| setNotificationCurrentState: setUpgradeNotificationCurrentState, | |||
| toggleSidebar: onToggleSidebar, | |||
| }} | |||
There was a problem hiding this comment.
Suggestion: the notificationCurrentState and setNotificationCurrentState in the PluginSlot pluginProps above can also be removed, correct?
There was a problem hiding this comment.
these are still needed since pluggable notification components use this as a callback to open or close the sidebar
| @@ -34,9 +33,7 @@ const UnitSuspense = ({ | |||
| pluginProps={{ | |||
| courseId, | |||
| }} | |||
| > | |||
| <LockPaywall courseId={courseId} /> | |||
There was a problem hiding this comment.
My instinct is that this should remain as it feels like a core feature of Open edX, but I'm guessing you're leaving here to confirm with Axim?
Zacharis278
changed the title
Zacharis278
force-pushed
the
zhancock/remove-upgrade-ads
branch
from
48309c7
to
397f6a7
Compare
|
@arbrandes as requested this is a follow up to remove unused code as part of openedx/platform-roadmap#332. I'll leave timing of merging this up to Axim since this has already been removed from edx.org |
|
@arbrandes is there still desire to have this removed as part of the 2U work? I'm clearing out this project on our end and this is the only open task. |
|
@Zacharis278 @arbrandes I would like to get this merged. Is there anything blocking merging it? An open question about |
Followup to #1368.
These components are specific to edx.org and will be moved to a 2U owned plugin. There are some open questions and I'd like to go back and manually test everything once the 2U plugin is updated to no longer import these. Leaving this in draft state for now.edx.org has migrated these components to a 2U owned plugin. This code is no longer being used and can be deprecated. While the LockPaywall is also unused we have opted not to remove it as originally intended. See note below
Removed by this PR
LockPaywall: Upgrade ad shown in place of graded unit if content gating is enabledNo longer included in this PRUpsellBullets: Only used by Upgrade/PaywallCannot be removed since we're keeping the LockPaywallNot Removed by this PR
Open Questions
Unsure on what to do with the LockPaywall. Should this actually be removed? It puts us in a weird state where 'nothing' would be displayed if content gating is enabled and that plugin slot isn't configured. I'm thinking this should either be reworked so the
shouldDisplayContentGatinglogic moves into the plugin itself or we don't touch LockPaywall at all here.Decision: We will not be removing the LockPaywall as part of this effort