Skip to content
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.

Sign up for GitHub

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: Added Plugin Slot wrapping UpgradeNotification in NotificationTray #1367

Merged
merged 1 commit into from
Apr 22, 2024
Merged

feat: Added Plugin Slot wrapping UpgradeNotification in NotificationTray #1367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 24 additions & 15 deletions src/courseware/course/sidebar/sidebars/notifications/NotificationTray.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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)}
Expand All @@ -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} />
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things:

  • The verifiedMode check should be removed, and instead delegated to pluginProps
  • The UpgradeNotification component should not be present by default
  • If there are no other uses of UpgradeNotification, the component itself should be removed from the codebase
  • The props are far too feature-specific; we should probably rely on global context in the future

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arbrandes can we move this conversation to open PR into main here since this was just a subset of the work? #1368

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course! I didn't realize this was a PR into a feature branch. :)

</PluginSlot>
) : (
<p className="p-3 small">{intl.formatMessage(messages.noNotificationsMessage)}</p>
)}
Expand Down
10 changes: 7 additions & 3 deletions src/courseware/course/sidebar/sidebars/notifications/NotificationTray.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,14 @@ describe('NotificationTray', () => {
<NotificationTray />
</SidebarContext.Provider>,
);
const UpgradeNotification = document.querySelector('.upgrade-notification');

expect(UpgradeNotification)
.toBeInTheDocument();
const pluginSlot = screen.getByTestId('notification-tray-slot');
expect(pluginSlot).toBeInTheDocument();

// The Upgrade Notification should be inside the PluginSlot.
const UpgradeNotification = pluginSlot.querySelector('.upgrade-notification');
expect(UpgradeNotification).toBeInTheDocument();

expect(screen.getByRole('link', { name: 'Upgrade for $149' }))
.toBeInTheDocument();
expect(screen.queryByText('You have no new notifications at this time.'))
Expand Down
Loading