feat: wrap existing sidebars in frontend-plugin-framework PluginSlots

[brian-smith-tcril]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.31%. Comparing base (642031b) to head (7453d94).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1543      +/-   ##
==========================================
+ Coverage   89.25%   89.31%   +0.06%     
==========================================
  Files         318      324       +6     
  Lines        5563     5595      +32     
  Branches     1379     1377       -2     
==========================================
+ Hits         4965     4997      +32     
+ Misses        583      582       -1     
- Partials       15       16       +1     

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

[bradenmacdonald]

Cool. The discussion here about providing courseId via standardized context is very relevant here too.

[bradenmacdonald]

But what is the "course outline sidebar mobile trigger"?

[brian-smith-tcril]

This ties in to the "wrapping complexity" goal of this PR. The course outline sidebar trigger (the button that opens up the sidebar) is displayed in a different location (both visually and within the DOM) on mobile vs on desktop. The CourseOutlineTrigger component is used in both Sequence and Course. In Course, the component is used with isMobileView always set to true, where as in Sequence it is always set to false.

With the new slots, CourseOutlineMobileSidebarTriggerSlot exists only in Course, and CourseOutlineSidebarTriggerSlot exists only in Sequence.

I'm not sure what the best way to communicate this complexity in the README would be, any suggestions are very warmly welcome!

[bradenmacdonald]

I see. I do think it's more clear with the updated README screenshots showing before+after! Otherwise I don't have any suggestions :/

[bradenmacdonald]

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:

[brian-smith-tcril]

I've updated all the READMEs to have 2 screenshots now. 1 showing default content and 1 showing custom content.

[xitij2000]

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.

[brian-smith-tcril]

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.

Great suggestion! I optimized them here https://github.com/openedx/frontend-app-learning/compare/5b453a31c244950d557c25d98b56445edaab4525..7453d94b1d5c8fdfe0b91acded9789c2465bc772

[bradenmacdonald]

Other than the ongoing discussion about how to handle context/props like courseId, I think this looks great. Thanks for adding those additional screenshots to the READMEs - it really helps!

[arbrandes]

Works for me! 👍🏼