refactor: plugin slot implementation #465
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #465 +/- ##
==========================================
+ Coverage 97.24% 97.39% +0.14%
==========================================
Files 154 156 +2
Lines 1378 1380 +2
Branches 228 228
==========================================
+ Hits 1340 1344 +4
+ Misses 36 34 -2
Partials 2 2 ☔ View full report in Codecov by Sentry. |
MaxFrank13
force-pushed
the
mfrank/plugin-slot-docs
branch
from
e2481ec to
9c97d4c
Compare
|
@sarina I have added some screenshots to the PR description for any of your documentation needs |
| @@ -38,14 +39,4 @@ export const CourseList = ({ | |||
| ); | |||
| }; | |||
|
|
|||
| CourseList.propTypes = { | |||
There was a problem hiding this comment.
I have no issue with removing propTypes, but this change is not obviously related to the plugin slot refactor. Would you mind adding it to a "changelog" in the PR comment?
There was a problem hiding this comment.
So I went back on this change. Originally I thought it made sense to separate the hook call into it's own component, but looking back at this, I think it's better to pass the data in as pluginProps. This way any custom components (like the example) don't need to call for the data themselves.
And yet, if a consumer wanted to do that (say they had a private API they wanted to get their course data from), they still can.
The only thing that changed here now is that courseListData is passed as one object instead of each value within courseListData being passed as individual props.
There was a problem hiding this comment.
@brian-smith-tcril do you have any opinions on this or are you cool with us proceeding with these changes?
There was a problem hiding this comment.
My only concern is that by adding something to pluginProps we're committing to it being a part of the API, so changing it in the future would be a breaking change and require communication. I'm not opposed to it, but it is worth considering.
One question I have is where is courseListDataShape being used? It seems in CourseListSlot courseListData is just PropTypes.shape()
There was a problem hiding this comment.
by adding something to pluginProps we're committing to it being a part of the API
Great point. My thinking here is that because it is the CourseListSlot, then passing the courseListData that is used in the default CourseList makes sense. Given that consumers would presumably need that same data for their custom implementation.
One question I have is where is courseListDataShape being used? It seems in CourseListSlot courseListData is just PropTypes.shape()
Good catch! I was thinking the courseListData in the CourseListSlot would import use that same courseListDataShape object but must have missed it
|
Drafted some light release notes. They're meant for non-technical audiences and do gloss over details, particularly just how flexible some slots are. Feel free to add comments. https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4505894933/Customizing+Site+Headers+Learner+Dashboard |
…-app-learner-dashboard into mfrank/plugin-slot-docs
Good callout! I added back in a test that wasn't moved over in the refactor and codecov is back to 100% |
This refactors PluginSlots for the
CourseList,NoCoursesView, andWidgetSidebarcomponents to match the pattern set in the repo whenFooterSlotwas added (source). This resolves #344.Changelog
The
CourseListcomponent has also been refactored to instead take in a singlecourseListDataprop rather than a prop for each field from the output of theuseCourseListDatahook.The name of the
WidgetSidebarSlotID has been changed fromwidget_sidebar_plugin_slottowidget_sidebar_slot.There were two wrappers on the
WidgetSidebarcomponent that have been removed as they weren't impacting the layout in any way. This was left over from a previous refactor that de-duplicated some old code #386The
LookingForChallengeWidgethas also been added as default content for theWidgetSidebarSlot. This was trivial enough to add in with this work and made sense as I had to document the slot anyway. This resolves #336.Screenshots
CourseListSlot
CourseList Default
Custom CourseList example that re-orients the cards
NoCoursesViewSlot
NoCoursesView default
Custom NoCoursesView example with a different call-to-action component
WidgetSidebarSlot
WidgetSidebar default
Custom Sidebar example with a different call-to-action component