feat: wrap existing sidebars in frontend-plugin-framework PluginSlots
#1543
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1543 +/- ##
==========================================
+ Coverage 89.25% 89.27% +0.02%
==========================================
Files 318 323 +5
Lines 5563 5577 +14
Branches 1379 1379
==========================================
+ Hits 4965 4979 +14
+ Misses 583 582 -1
- Partials 15 16 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Cool. The discussion here about providing courseId via standardized context is very relevant here too.
|
|
||
| ## Description | ||
|
|
||
| This slot is used to replace/modify/hide the course outline sidebar mobile trigger. |
There was a problem hiding this comment.
But what is the "course outline sidebar mobile trigger"?
|
|
||
| ## Example | ||
|
|
||
|  |
There was a problem hiding this comment.
These screenshots with some 📊 in the slot are kind of helpful, but it would also be helpful to see what the default content looks like. If we had to have only one screenshot, I think I would prefer something like this:
|
|
||
| ## Description | ||
|
|
||
| This slot is used to replace/modify/hide the course outline sidebar. |
There was a problem hiding this comment.
I think it's worth mentioning that this slot is only visible if the courseware.enable_navigation_sidebar waffle flag is enabled.
| }; | ||
|
|
||
| NotificationsDiscussionsSidebarSlot.propTypes = { | ||
| courseId: PropTypes.string.isRequired, |
There was a problem hiding this comment.
Can we please use .tsx and TypeScript types instead of .jsx and propTypes for this sort of net new component?
| courseId: PropTypes.string.isRequired, | ||
| }; | ||
|
|
||
| export default NotificationsDiscussionsSidebarSlot; |
There was a problem hiding this comment.
Nit: with the latest frontend-build, you don't have to use default exports, but you can if you want to.
|
Just one small nit. I think we can save some file size with not quality loss by running these png files through something like oxipng. I did so on my PR and it saved around 30-40% on file sizes. They are small files but over time with a lot of images it'll add up. |
This PR is an MVP implementation of slots for the sidebars. The sidebars have grown quite complex over time, so the goal of this PR is to "wrap the complexity" and ensure everything sidebar related is wrapped in a slot.
The existing show/hide logic is still intact inside the slots, so the slots will not be shown/hidden by the existing sidebar triggers. This was an intentional decision to avoid coupling the new slots to existing sidebar logic.
My hope is that once this PR lands we'll have a good jumping off point for discussions around how to architect the MFE to support simple and reasonable built-in sidebars as well as anything site operators want to build.