-
Notifications
You must be signed in to change notification settings - Fork 86
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
Merged
Merged
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
9c97d4c
refactor: plugin slots implementation
MaxFrank13 a9975f7
refactor: plugin docs
MaxFrank13 606f03e
refactor: plugin docs
MaxFrank13 cbdaaef
refactor: docs
MaxFrank13 9b16481
refactor: docs
MaxFrank13 9d4361a
refactor: added back props to CourseList
MaxFrank13 d90737f
refactor: prop-types for CourstList
MaxFrank13 354db91
refactor: readme tabs to spaces
MaxFrank13 667331b
refactor: CourseListSlot props
MaxFrank13 40d9b0d
Merge branch 'master' into mfrank/plugin-slot-docs
MaxFrank13 6c1bdf6
refactor: WidgetSidebarSlot test
MaxFrank13 b2477bc
Merge branch 'master' into mfrank/plugin-slot-docs
MaxFrank13 3964980
Merge branch 'mfrank/plugin-slot-docs' of github.com:openedx/frontend…
MaxFrank13 c6af1a5
refactor: courseListDataShape for slot
MaxFrank13 61ffc99
Merge branch 'master' into mfrank/plugin-slot-docs
MaxFrank13 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
15 changes: 0 additions & 15 deletions
15
src/containers/WidgetContainers/WidgetSidebar/__snapshots__/index.test.jsx.snap
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
18 changes: 0 additions & 18 deletions
18
src/containers/WidgetContainers/WidgetSidebar/index.test.jsx
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
# Course List Slot | ||
|
||
### Slot ID: `course_list_slot` | ||
|
||
## Plugin Props | ||
|
||
* courseListData | ||
|
||
## Description | ||
|
||
This slot is used for replacing or adding content around the `CourseList` component. The `CourseListSlot` is only rendered if the learner has enrolled in at least one course. | ||
|
||
## Example | ||
|
||
The space will show the `CourseList` component by default. This can be disabled in the configuration with the `keepDefault` boolean. | ||
|
||
![Screenshot of the CourseListSlot](./images/course_list_slot.png) | ||
|
||
Setting the MFE's `env.config.jsx` to the following will replace the default experience with a list of course titles. | ||
|
||
![Screenshot of a custom course list](./images/readme_custom_course_list.png) | ||
|
||
```js | ||
import { DIRECT_PLUGIN, PLUGIN_OPERATIONS } from '@openedx/frontend-plugin-framework'; | ||
|
||
const config = { | ||
pluginSlots: { | ||
course_list_slot: { | ||
// Hide the default CourseList component | ||
keepDefault: false, | ||
plugins: [ | ||
{ | ||
op: PLUGIN_OPERATIONS.Insert, | ||
widget: { | ||
id: 'custom_course_list', | ||
type: DIRECT_PLUGIN, | ||
priority: 60, | ||
RenderWidget: ({ courseListData }) => { | ||
// Extract the "visibleList" | ||
const courses = courseListData.visibleList; | ||
// Render a list of course names | ||
return ( | ||
<div> | ||
{courses.map(courseData => ( | ||
<p> | ||
{courseData.course.courseName} | ||
</p> | ||
))} | ||
</div> | ||
) | ||
}, | ||
}, | ||
}, | ||
], | ||
}, | ||
}, | ||
} | ||
|
||
export default config; | ||
``` |
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
|
||
import { PluginSlot } from '@openedx/frontend-plugin-framework'; | ||
import { CourseList } from 'containers/CoursesPanel/CourseList'; | ||
|
||
export const CourseListSlot = ({ courseListData }) => ( | ||
<PluginSlot id="course_list_slot" pluginProps={{ courseListData }}> | ||
<CourseList courseListData={courseListData} /> | ||
</PluginSlot> | ||
); | ||
|
||
CourseListSlot.propTypes = { | ||
courseListData: PropTypes.shape().isRequired, | ||
}; | ||
|
||
export default CourseListSlot; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
# No Courses View Slot | ||
|
||
### Slot ID: `no_courses_view_slot` | ||
|
||
## Description | ||
|
||
This slot is used for replacing or adding content around the `NoCoursesView` component. The `NoCoursesViewSlot` only renders if the learner has not yet enrolled in any courses. | ||
|
||
## Example | ||
|
||
The space will show the `NoCoursesView` by default. This can be disabled in the configuration with the `keepDefault` boolean. | ||
|
||
![Screenshot of the no courses view](./images/no_courses_view_slot.png) | ||
|
||
Setting the MFE's `env.config.jsx` to the following will replace the default experience with a custom call-to-action component. | ||
|
||
![Screenshot of a custom no courses view](./images/readme_custom_no_courses_view.png) | ||
|
||
```js | ||
import { DIRECT_PLUGIN, PLUGIN_OPERATIONS } from '@openedx/frontend-plugin-framework'; | ||
|
||
const config = { | ||
pluginSlots: { | ||
no_courses_view_slot: { | ||
// Hide the default NoCoursesView component | ||
keepDefault: false, | ||
plugins: [ | ||
{ | ||
op: PLUGIN_OPERATIONS.Insert, | ||
widget: { | ||
id: 'custom_no_courses_CTA', | ||
type: DIRECT_PLUGIN, | ||
priority: 60, | ||
RenderWidget: () => ( | ||
<h3> | ||
Check out our catalog of courses and start learning today! | ||
</h3> | ||
), | ||
}, | ||
}, | ||
], | ||
}, | ||
}, | ||
} | ||
|
||
export default config; | ||
``` |
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added
BIN
+23.9 KB
src/plugin-slots/NoCoursesViewSlot/images/readme_custom_no_courses.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import React from 'react'; | ||
|
||
import { PluginSlot } from '@openedx/frontend-plugin-framework'; | ||
import NoCoursesView from 'containers/CoursesPanel/NoCoursesView'; | ||
|
||
export const NoCoursesViewSlot = () => ( | ||
<PluginSlot id="no_courses_view_slot"> | ||
<NoCoursesView /> | ||
</PluginSlot> | ||
); | ||
|
||
export default NoCoursesViewSlot; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
# `frontend-app-learner-dashboard` Plugin Slots | ||
|
||
* [`footer_slot`](./FooterSlot/) | ||
* [`widget_sidebar_slot`](./WidgetSidebarSlot/) | ||
* [`course_list_slot`](./CourseListSlot/) | ||
* [`no_courses_view_slot`](./NoCoursesViewSlot/) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 withincourseListData
being passed as individual props.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inCourseListSlot
courseListData
is justPropTypes.shape()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point. My thinking here is that because it is the
CourseListSlot
, then passing thecourseListData
that is used in the defaultCourseList
makes sense. Given that consumers would presumably need that same data for their custom implementation.Good catch! I was thinking the
courseListData
in theCourseListSlot
would import use that samecourseListDataShape
object but must have missed it