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

refactor: plugin slot implementation #465

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

MaxFrank13
Copy link
Member

@MaxFrank13 MaxFrank13 commented Sep 25, 2024

This refactors PluginSlots for the CourseList, NoCoursesView, and WidgetSidebar components to match the pattern set in the repo when FooterSlot was added (source). This resolves #344.

Changelog

The CourseList component has also been refactored to instead take in a single courseListData prop rather than a prop for each field from the output of the useCourseListData hook.

There were two wrappers on the WidgetSidebar component 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 #386

The LookingForChallengeWidget has also been added as default content for the WidgetSidebarSlot. 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

Screenshot 2024-09-26 at 3 10 30 PM

Custom CourseList example that re-orients the cards

custom_course_list

NoCoursesViewSlot

NoCoursesView default

no_courses_view_slot_open

Custom NoCoursesView example with a different call-to-action component

custom_no_courses_view

WidgetSidebarSlot

WidgetSidebar default

widget_sidebar_slot_open_02

Custom Sidebar example with a different call-to-action component

custom_CTA_sidebar

Copy link

codecov bot commented Sep 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.39%. Comparing base (cc544e4) to head (3964980).

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      229      227       -2     
==========================================
+ Hits         1340     1344       +4     
+ Misses         36       34       -2     
  Partials        2        2              

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

@MaxFrank13 MaxFrank13 marked this pull request as ready for review September 26, 2024 13:40
@MaxFrank13 MaxFrank13 requested a review from a team as a code owner September 26, 2024 13:40
@MaxFrank13
Copy link
Member Author

@sarina I have added some screenshots to the PR description for any of your documentation needs

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

Overall this looks awesome! Super happy to see slots in components becoming the standard!

I left a few comments in there but nothing major.

Thank you so much for doing this!

src/plugin-slots/CourseListSlot/README.md Outdated Show resolved Hide resolved
@@ -38,14 +39,4 @@ export const CourseList = ({
);
};

CourseList.propTypes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@MaxFrank13 MaxFrank13 Oct 3, 2024

Choose a reason for hiding this comment

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

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.

src/containers/CoursesPanel/index.jsx Outdated Show resolved Hide resolved
src/plugin-slots/NoCoursesViewSlot/README.md Outdated Show resolved Hide resolved
src/plugin-slots/WidgetSidebarSlot/README.md Outdated Show resolved Hide resolved
src/plugin-slots/WidgetSidebarSlot/index.jsx Outdated Show resolved Hide resolved
src/plugin-slots/WidgetSidebarSlot/index.jsx Outdated Show resolved Hide resolved
@sarina
Copy link

sarina commented Sep 30, 2024

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

Copy link
Member

@deborahgu deborahgu left a comment

Choose a reason for hiding this comment

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

this looks good to me. Are we concerned about the codecov warning?

@MaxFrank13
Copy link
Member Author

this looks good to me. Are we concerned about the codecov warning?

Good callout! I added back in a test that wasn't moved over in the refactor and codecov is back to 100%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Create components for PluginSlots, standardize id naming Reimplement "Looking for a new challenge" panel
4 participants