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 14 commits into
base: master
Choose a base branch
from
Open
19 changes: 5 additions & 14 deletions src/containers/CoursesPanel/CourseList/index.jsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React from 'react';
import PropTypes from 'prop-types';

import { Pagination } from '@openedx/paragon';
import {
Expand All @@ -8,11 +7,13 @@ import {
import CourseCard from 'containers/CourseCard';

import { useIsCollapsed } from './hooks';
import { useCourseListData } from '../hooks';

export const CourseList = ({
filterOptions, setPageNumber, numPages, showFilters, visibleList,
}) => {
export const CourseList = () => {
const isCollapsed = useIsCollapsed();
const {
filterOptions, setPageNumber, numPages, showFilters, visibleList,
} = useCourseListData();
return (
<>
{showFilters && (
Expand All @@ -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.

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

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,
numPages: PropTypes.number.isRequired,
setPageNumber: PropTypes.func.isRequired,
};

export default CourseList;
14 changes: 11 additions & 3 deletions src/containers/CoursesPanel/CourseList/index.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,16 @@ import { shallow } from '@edx/react-unit-test-utils';

import { useIsCollapsed } from './hooks';
import CourseList from '.';
import { useCourseListData } from '../hooks';

jest.mock('./hooks', () => ({
useIsCollapsed: jest.fn(),
}));

jest.mock('../hooks', () => ({
useCourseListData: jest.fn(),
}));

jest.mock('containers/CourseCard', () => 'CourseCard');
jest.mock('containers/CourseFilterControls', () => ({
ActiveCourseFilters: 'ActiveCourseFilters',
Expand All @@ -22,9 +27,12 @@ describe('CourseList', () => {
};
useIsCollapsed.mockReturnValue(false);

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

describe('no courses or filters', () => {
test('snapshot', () => {
Expand Down
18 changes: 2 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,6 @@ 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 />
</div>
`;
18 changes: 4 additions & 14 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 @@ -35,17 +33,9 @@ export const CoursesPanel = () => {
</div>
</div>
{hasCourses ? (
<PluginSlot
id="course_list"
>
<CourseList {...courseListData} />
</PluginSlot>
<CourseListSlot />
) : (
<PluginSlot
id="no_courses_view"
>
<NoCoursesView />
</PluginSlot>
<NoCoursesViewSlot />
MaxFrank13 marked this conversation as resolved.
Show resolved Hide resolved
)}
</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

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.

43 changes: 43 additions & 0 deletions src/plugin-slots/CourseListSlot/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Course List Slot

### Slot ID: `course_list_slot`

## 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 `CustomCourseList` component.

![Screenshot of a custom course list](./images/custom_course_list.png)

```js
import { DIRECT_PLUGIN, PLUGIN_OPERATIONS } from '@openedx/frontend-plugin-framework';
import { CustomCourseList } from '<package-that-exports-your-component>'
MaxFrank13 marked this conversation as resolved.
Show resolved Hide resolved

const config = {
pluginSlots: {
course_list_slot: {
keepDefault: false,
plugins: [
{
op: ops.Insert,
widget: {
id: 'custom_course_list',
type: DIRECT_PLUGIN,
priority: 60,
RenderWidget: CustomCourseList,
},
},
],
},
},
}

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.
11 changes: 11 additions & 0 deletions src/plugin-slots/CourseListSlot/index.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import React from 'react';

import { PluginSlot } from '@openedx/frontend-plugin-framework';
import CourseList from 'containers/CoursesPanel/CourseList';

export const CourseListSlot = () => (
<PluginSlot id="course_list_slot">
<CourseList />
</PluginSlot>
);
export default CourseListSlot;
43 changes: 43 additions & 0 deletions src/plugin-slots/NoCoursesViewSlot/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# 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 `CustomNoCoursesCTA` component.

![Screenshot of a custom no courses view](./images/custom_no_courses_view.png)

```js
import { DIRECT_PLUGIN, PLUGIN_OPERATIONS } from '@openedx/frontend-plugin-framework';
import { CustomSidebarPanel } from 'package-that-exports-your-component';
MaxFrank13 marked this conversation as resolved.
Show resolved Hide resolved

const config = {
pluginSlots: {
no_courses_view_slot: {
keepDefault: false,
plugins: [
{
op: ops.Insert,
widget: {
id: 'custom_no_courses_CTA',
type: DIRECT_PLUGIN,
priority: 60,
RenderWidget: CustomSidebarPanel,
},
},
],
},
},
}

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/)
43 changes: 43 additions & 0 deletions src/plugin-slots/WidgetSidebarSlot/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# Widget Sidebar Slot

### Slot ID: `widget_sidebar_slot`

## Description

This slot is used for adding content to the right-hand sidebar.

## Example

The space will show the `LookingForChallengeWidget` by default. This can be disabled in the configuration with the `keepDefault` boolean.

![Screenshot of the widget sidebar](./images/looking_for_challenge_widget.png)

Setting the MFE's `env.config.jsx` to the following will replace the default experience with a `CustomSidebarPanel` component.

![Screenshot of a custom call-to-action in the sidebar](./images/custom_CTA_sidebar.png)
MaxFrank13 marked this conversation as resolved.
Show resolved Hide resolved

```js
import { DIRECT_PLUGIN, PLUGIN_OPERATIONS } from '@openedx/frontend-plugin-framework';
import { CustomSidebarPanel } from 'package-that-exports-your-component';
MaxFrank13 marked this conversation as resolved.
Show resolved Hide resolved

const config = {
pluginSlots: {
widget_sidebar_slot: {
keepDefault: false,
plugins: [
{
op: ops.Insert,
widget: {
id: 'custom_sidebar_panel',
type: DIRECT_PLUGIN,
priority: 60,
RenderWidget: CustomSidebarPanel,
},
},
],
},
},
}

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.
13 changes: 13 additions & 0 deletions src/plugin-slots/WidgetSidebarSlot/index.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React from 'react';

import { PluginSlot } from '@openedx/frontend-plugin-framework';
import LookingForChallengeWidget from 'widgets/LookingForChallengeWidget';

// eslint-disable-next-line arrow-body-style
export const WidgetSidebar = () => (
MaxFrank13 marked this conversation as resolved.
Show resolved Hide resolved
<PluginSlot id="widget_sidebar_plugin_slot">

Check warning on line 8 in src/plugin-slots/WidgetSidebarSlot/index.jsx

View check run for this annotation

Codecov / codecov/patch

src/plugin-slots/WidgetSidebarSlot/index.jsx#L8

Added line #L8 was not covered by tests
<LookingForChallengeWidget />
</PluginSlot>
);

export default WidgetSidebar;
MaxFrank13 marked this conversation as resolved.
Show resolved Hide resolved
Loading