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.

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: generic sidebar notification plugin #1379

Merged

Conversation

zacharis278
Copy link
Contributor

@zacharis278 zacharis278 commented May 2, 2024

This is just a POC to prove out the API of params we're passing I didn't touch the new sidebar or write any tests.

Removes upgrade component specific properties from sidebar/notification plugin slots.

Since we're merging into the feature branch this would be the resulting diff against main: master...zhancock/generic-notification-plugin

Risks/Concerns
This component is still highly coupled with the state of the learning MFE that detail is just hidden into the component now. If the redux store changes structure it could break our plugin. Once this is working it would be good to evaluate the component code we've copied over, there's probably a lot of conditions we can carve out if the upgrade component is 2U specific.

Pairs with https://github.com/edx/frontend-plugin-advertisements/pull/10
^^ in case this isn't publicly visible the approach is to useModel and get all of these fields rather than passing them over a pluggable API.

eg

  const {
    accessExpiration,
    marketingUrl,
    offer,
    timeOffsetMillis,
  } = useModel('coursewareMeta', courseId);

>
<UpgradeNotification {...upgradeNotificationProps} />
<UpgradeNotification
Copy link
Contributor Author

Choose a reason for hiding this comment

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

revert back to what's in main today, no change in behavior if the plugin is off.

src/course-home/outline-tab/OutlineTab.jsx Outdated Show resolved Hide resolved
testId="notification-tray-slot"
pluginProps={{
courseId,
notificationCurrentState: upgradeNotificationCurrentState,
Copy link
Contributor Author

@zacharis278 zacharis278 May 2, 2024

Choose a reason for hiding this comment

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

This drives the 'red dot' notification icon. I've removed the 'upgrade' from how this is passed to the plugin to keep it general. On the learning MFE side all the variables relating to this are still called 'upgrade...' we should rename all of that eventually. Probably at the same time we remove all the upgrade code from the learning MFE.

I think these would belong on the API for this plugin slot since any plugin here may want to drive that icon. That said, the code for the red dot is tightly coupled to upgrade right now but you could use it creatively for other purposes. It would probably take further consideration on how we handle notifications from multiple sources (if you put more than 1 plugin here) when the need arises. My thinking is the existence of a getter/setter wouldn't need to change so what I have here should be a stable interface.

@arbrandes
Copy link

the approach is to useModel and get all of these fields rather than passing them over a pluggable API.

For better or worse, I think that will be inevitable in some of the first cases (like this one). This will still help guide how to build out an API that we can consider stable, though.


return <div data-testid={testId}>{children}</div>;
};
const MockedPluginSlot = ({ children, id }) => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to modify how this works a bit because we get react errors if testId is passed to the PluginSlot component. We can only pass id, pluginProps, and children. I opted to use id instead here since that would be required to have a functional plugin slot.

@@ -80,7 +80,7 @@ describe('NotificationsWidget', () => {
});
});

it('renders upgrade card', async () => {
it('includes notification_widget_plugin slot', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for this and other components I split testing the plugin slot and behavior of the default content w/o a plugin since we expect the latter to be removed.

Copy link

codecov bot commented May 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.27%. Comparing base (1e29715) to head (eba6583).

Additional details and impacted files
@@                      Coverage Diff                      @@
##           feat/upgrade-plugin-slots    #1379      +/-   ##
=============================================================
- Coverage                      88.30%   88.27%   -0.03%     
=============================================================
  Files                            293      293              
  Lines                           5009     5008       -1     
  Branches                        1267     1237      -30     
=============================================================
- Hits                            4423     4421       -2     
- Misses                           570      571       +1     
  Partials                          16       16              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

id="outline_tab"
pluginProps={upgradeNotificationProps}
testId="outline-tab-slot"
id="outline_tab_notifications_plugin"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the name on these to align convention with existing plugins xxxxx_plugin

@zacharis278 zacharis278 force-pushed the zhancock/generic-notification-plugin branch from efafc19 to eba6583 Compare May 7, 2024 13:24
Copy link
Member

@varshamenon4 varshamenon4 left a comment

Choose a reason for hiding this comment

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

lgtm!

@zacharis278 zacharis278 merged commit 0989559 into feat/upgrade-plugin-slots May 7, 2024
6 of 7 checks passed
@zacharis278 zacharis278 deleted the zhancock/generic-notification-plugin branch May 7, 2024 18:22
schenedx pushed a commit that referenced this pull request May 13, 2024
* feat: add plugin slot for fbe lock paywall (#1347)

* feat: Added PluginSlot wrapping UpgradeNotification components (#1366)

* chore: Updated PluginSlot mock to support children and test ids

* chore: Updated mocked PluginSlot

* chore: Added unit test for MockedPluginSlot

* fix: Updated slot name ids

* feat: Added Plugin Slot wrapping UpgradeNotification in NotificationTray (#1367)

* fix: Removed PluginSlot prop scoping for UpgradeNotification (#1369)

* feat: generic sidebar notification plugin (#1379)

* feat: make notification plugin api generic

* feat: include new sidebar and tests

* feat: tweak sidebar toggle

* style: fix extra space from merge

* feat: rename plugin slots

---------

Co-authored-by: Alison Langston <[email protected]>
Co-authored-by: Zachary Hancock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants