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

Merged
merged 15 commits into from
Oct 15, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions src/containers/CoursesPanel/CourseList/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ import CourseCard from 'containers/CourseCard';

import { useIsCollapsed } from './hooks';

export const CourseList = ({
filterOptions, setPageNumber, numPages, showFilters, visibleList,
}) => {
export const CourseList = ({ courseListData }) => {
const {
filterOptions, setPageNumber, numPages, showFilters, visibleList,
} = courseListData;
const isCollapsed = useIsCollapsed();
return (
<>
Expand All @@ -38,14 +39,16 @@ 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.

Copy link
Member Author

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?

Copy link
Contributor

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 in CourseListSlot courseListData is just PropTypes.shape()

Copy link
Member Author

Choose a reason for hiding this comment

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

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

export const courseListDataShape = PropTypes.shape({
showFilters: PropTypes.bool.isRequired,
// eslint-disable-next-line react/forbid-prop-types
visibleList: PropTypes.arrayOf(PropTypes.object).isRequired,
// eslint-disable-next-line react/forbid-prop-types
filterOptions: PropTypes.object.isRequired,
visibleList: PropTypes.arrayOf(PropTypes.shape()).isRequired,
filterOptions: PropTypes.shape().isRequired,
numPages: PropTypes.number.isRequired,
setPageNumber: PropTypes.func.isRequired,
});

CourseList.propTypes = {
courseListData: courseListDataShape,
};

export default CourseList;
2 changes: 1 addition & 1 deletion src/containers/CoursesPanel/CourseList/index.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('CourseList', () => {
useIsCollapsed.mockReturnValue(false);

const createWrapper = (courseListData = defaultCourseListData) => (
shallow(<CourseList {...courseListData} />)
shallow(<CourseList courseListData={courseListData} />)
);

describe('no courses or filters', () => {
Expand Down
28 changes: 12 additions & 16 deletions src/containers/CoursesPanel/__snapshots__/index.test.jsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@ exports[`CoursesPanel no courses snapshot 1`] = `
<CourseFilterControls />
</div>
</div>
<PluginSlot
id="no_courses_view"
>
<NoCoursesView />
</PluginSlot>
<NoCoursesViewSlot />
</div>
`;

Expand All @@ -44,16 +40,16 @@ exports[`CoursesPanel with courses snapshot 1`] = `
<CourseFilterControls />
</div>
</div>
<PluginSlot
id="course_list"
>
<CourseList
filterOptions={{}}
numPages={1}
setPageNumber={[MockFunction setPageNumber]}
showFilters={false}
visibleList={[]}
/>
</PluginSlot>
<CourseListSlot
courseListData={
{
"filterOptions": {},
"numPages": 1,
"setPageNumber": [MockFunction setPageNumber],
"showFilters": false,
"visibleList": [],
}
}
/>
</div>
`;
20 changes: 3 additions & 17 deletions src/containers/CoursesPanel/index.jsx
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
import React from 'react';

import { PluginSlot } from '@openedx/frontend-plugin-framework';
import { useIntl } from '@edx/frontend-platform/i18n';

import { reduxHooks } from 'hooks';
import {
CourseFilterControls,
} from 'containers/CourseFilterControls';
import NoCoursesView from './NoCoursesView';

import CourseList from './CourseList';
import CourseListSlot from 'plugin-slots/CourseListSlot';
import NoCoursesViewSlot from 'plugin-slots/NoCoursesViewSlot';

import { useCourseListData } from './hooks';

Expand All @@ -34,19 +32,7 @@ export const CoursesPanel = () => {
<CourseFilterControls {...courseListData.filterOptions} />
</div>
</div>
{hasCourses ? (
<PluginSlot
id="course_list"
>
<CourseList {...courseListData} />
</PluginSlot>
) : (
<PluginSlot
id="no_courses_view"
>
<NoCoursesView />
</PluginSlot>
)}
{hasCourses ? <CourseListSlot courseListData={courseListData} /> : <NoCoursesViewSlot />}
</div>
);
};
Expand Down
4 changes: 2 additions & 2 deletions src/containers/Dashboard/DashboardLayout.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import PropTypes from 'prop-types';

import { Container, Col, Row } from '@openedx/paragon';

import WidgetSidebar from '../WidgetContainers/WidgetSidebar';
import WidgetSidebarSlot from 'plugin-slots/WidgetSidebarSlot';

import hooks from './hooks';

Expand Down Expand Up @@ -42,7 +42,7 @@ export const DashboardLayout = ({ children }) => {
</Col>
<Col {...columnConfig.sidebar} className="sidebar-column">
{!isCollapsed && (<h2 className="course-list-title">&nbsp;</h2>)}
<WidgetSidebar />
<WidgetSidebarSlot />
</Col>
</Row>
</Container>
Expand Down
4 changes: 2 additions & 2 deletions src/containers/Dashboard/DashboardLayout.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ describe('DashboardLayout', () => {
const columns = el.instance.findByType(Row)[0].findByType(Col);
expect(columns[0].children).not.toHaveLength(0);
});
it('displays WidgetSidebar in second column', () => {
it('displays WidgetSidebarSlot in second column', () => {
const columns = el.instance.findByType(Row)[0].findByType(Col);
expect(columns[1].findByType('WidgetSidebar')).toHaveLength(1);
expect(columns[1].findByType('WidgetSidebarSlot')).toHaveLength(1);
});
};
const testSidebarLayout = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ exports[`DashboardLayout collapsed sidebar not showing snapshot 1`] = `
}
}
>
<WidgetSidebar />
<WidgetSidebarSlot />
</Col>
</Row>
</Container>
Expand Down Expand Up @@ -82,7 +82,7 @@ exports[`DashboardLayout collapsed sidebar showing snapshot 1`] = `
}
}
>
<WidgetSidebar />
<WidgetSidebarSlot />
</Col>
</Row>
</Container>
Expand Down Expand Up @@ -131,7 +131,7 @@ exports[`DashboardLayout not collapsed sidebar not showing snapshot 1`] = `
>

</h2>
<WidgetSidebar />
<WidgetSidebarSlot />
</Col>
</Row>
</Container>
Expand Down Expand Up @@ -180,7 +180,7 @@ exports[`DashboardLayout not collapsed sidebar showing snapshot 1`] = `
>

</h2>
<WidgetSidebar />
<WidgetSidebarSlot />
</Col>
</Row>
</Container>
Expand Down

This file was deleted.

23 changes: 0 additions & 23 deletions src/containers/WidgetContainers/WidgetSidebar/index.jsx

This file was deleted.

18 changes: 0 additions & 18 deletions src/containers/WidgetContainers/WidgetSidebar/index.test.jsx

This file was deleted.

60 changes: 60 additions & 0 deletions src/plugin-slots/CourseListSlot/README.md
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.
17 changes: 17 additions & 0 deletions src/plugin-slots/CourseListSlot/index.jsx
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;
47 changes: 47 additions & 0 deletions src/plugin-slots/NoCoursesViewSlot/README.md
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.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
12 changes: 12 additions & 0 deletions src/plugin-slots/NoCoursesViewSlot/index.jsx
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;
3 changes: 3 additions & 0 deletions src/plugin-slots/README.md
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/)
Loading