Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 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
57 changes: 4 additions & 53 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,9 @@ Development Setup

import { PLUGIN_OPERATIONS, DIRECT_PLUGIN } from "@openedx/frontend-plugin-framework";
import {
SidebarToggleWrapper,
CourseHeaderButton,
UnitActionsButton,
AspectsSidebarProvider,
CourseOutlineSidebar,
UnitPageSidebar,
CourseOutlineSidebarWrapper,
UnitOutlineSidebarWrapper,
SubSectionAnalyticsButton,
} from "@openedx/frontend-plugin-aspects";

Expand All @@ -83,66 +80,20 @@ Development Setup
"org.openedx.frontend.authoring.course_outline_sidebar.v1": {
keepDefault: true,
plugins: [
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: "outline-sidebar",
priority: 1,
type: DIRECT_PLUGIN,
RenderWidget: CourseOutlineSidebar,
},
},
{
op: PLUGIN_OPERATIONS.Wrap,
widgetId: "default_contents",
wrapper: SidebarToggleWrapper,
wrapper: CourseOutlineSidebarWrapper,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@rpenido I have a question here. From what I understand in the changes in this README, the sidebar was previously used with the Insert operation. Now it no longer uses the Insert operation but the Wrapper operation. What happens to the instances that previously used Insert? These are breaking changes, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What happens to the instances that previously used Insert?

They will be inserted before the new sidebar.

These are breaking changes, right?

I think they may be breaking changes on the authoring side. In this PR, we are just fixing the plugin that uses the slot to add a new page, instead of inserting it before the sidebar.

},
],
},
"org.openedx.frontend.authoring.course_unit_sidebar.v2": {
keepDefault: true,
plugins: [
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: "course-unit-sidebar",
priority: 1,
type: DIRECT_PLUGIN,
RenderWidget: UnitPageSidebar,
},
},
{
op: PLUGIN_OPERATIONS.Wrap,
widgetId: "default_contents",
wrapper: SidebarToggleWrapper,
},
],
},
"org.openedx.frontend.authoring.course_outline_header_actions.v1": {
keepDefault: true,
plugins: [
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: "outline-analytics",
type: DIRECT_PLUGIN,
priority: 51,
RenderWidget: CourseHeaderButton,
},
},
],
},
"org.openedx.frontend.authoring.course_unit_header_actions.v1": {
keepDefault: true,
plugins: [
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: "unit-analytics",
type: DIRECT_PLUGIN,
priority: 51,
RenderWidget: CourseHeaderButton,
},
wrapper: UnitOutlineSidebarWrapper,
},
],
},
Expand Down
1 change: 1 addition & 0 deletions module.config.js.example
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ module.exports = {
// { moduleName: '@openedx/paragon/icons', dir: '../paragon', dist: 'icons' },
// { moduleName: '@openedx/paragon', dir: '../paragon', dist: 'dist' },
// { moduleName: '@edx/frontend-platform', dir: '../frontend-platform', dist: 'dist' },
// { moduleName: 'CourseAuthoring', dir: '../src', dist: 'dist' },
],
};
9 changes: 4 additions & 5 deletions src/components/AspectsSidebar/CourseContentList.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as React from 'react';
import React from 'react';
import { useIntl } from '@edx/frontend-platform/i18n';
import { Button, Icon } from '@openedx/paragon';
import { ArrowDropDown, ArrowDropUp, ChevronRight } from '@openedx/paragon/icons';
Expand All @@ -7,25 +7,24 @@ import { ICON_MAP } from '../../constants';
import { Block } from '../../types';

interface CourseContentListProps {
title: string,
blocks: Block[],
activateDashboard: (block: Block) => void,
}

export function CourseContentList({
title, blocks, activateDashboard,
blocks, activateDashboard,
}: CourseContentListProps) {
const intl = useIntl();
// using undefined is useful for slicing the list
const [showCount, setShowCount] = React.useState<number | undefined>(5);

// istanbul ignore next: this should never happen, as the component is only rendered when there are blocks
if (!blocks.length) {
return null;
}

return (
<div className="d-flex flex-column rounded-bottom py-4 px-3 bg-white border-top border-light">
{!!title && <h4 className="h4 mb-4">{title}</h4>}
<div className="d-flex flex-column rounded-bottom py-4 px-3 bg-white">
{blocks?.slice(0, showCount).map(block => (
<Button
key={block.id}
Expand Down
8 changes: 4 additions & 4 deletions src/components/AspectsSidebar/Dashboard.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as React from 'react';
import React from 'react';
import { useParams } from 'react-router-dom';
import { embedDashboard, EmbeddedDashboard, Size } from '@superset-ui/embedded-sdk';
import {
Expand All @@ -12,7 +12,7 @@ import '../../styles.css';
import messages from '../../messages';

function Embed({ usageKey }: { usageKey: string }) {
const { loading, error: configError, config: dashboardConfig } = useDashboardConfig(usageKey);
const { isPending, error: configError, data: dashboardConfig } = useDashboardConfig(usageKey);
const containerDiv = React.useRef(null);
const { courseId } = useParams();
const [containerHeight, setContainerHeight] = React.useState<number>(0);
Expand Down Expand Up @@ -81,14 +81,14 @@ function Embed({ usageKey }: { usageKey: string }) {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [dashboardConfig, courseId]);

if (loading) {
if (isPending) {
return <Skeleton />;
}

if (configError) {
return (
<Alert variant="danger">
{configError}
{configError.toString()}
</Alert>
);
}
Expand Down
52 changes: 23 additions & 29 deletions src/components/AspectsSidebar/index.test.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import * as React from 'react';
import React from 'react';
import { fireEvent, render, screen } from '@testing-library/react';
import { BlockTypes } from '../../constants';
import { AspectsSidebar, ContentList } from '.';
import { AspectsSidebar, type ContentList } from '.';
import { useAspectsSidebarContext } from '../../hooks';
import messages from '../../messages';
import '@testing-library/jest-dom';
import { Block } from '../../types';

// Mock dependencies
Expand All @@ -15,6 +14,22 @@ jest.mock('@edx/frontend-platform/i18n', () => ({
}),
}));

/* eslint-disable react/prop-types */
jest.mock(
'CourseAuthoring/generic/sidebar',
() => ({
SidebarContent: ({ children }) => <div>{children}</div>,
SidebarSection: ({ title, children }) => <div><div>{title}</div>{children}</div>,
SidebarTitle: ({ title, onBackBtnClick }) => (
<div>
{onBackBtnClick && <button type="button" onClick={onBackBtnClick}>Back</button>}
{title}
</div>
),
}),
{ virtual: true },
);

const mockDashboard = jest.fn();
jest.mock('./Dashboard', () => ({
Dashboard: () => mockDashboard,
Expand Down Expand Up @@ -71,25 +86,19 @@ const defaultProps = {
};

type InitialState = {
sidebarOpen: boolean,
activeBlock?: Block | null,
filterUnit?: Block | null,
filteredBlocks?: string[]
};

// Helper component to set context values for tests
function TestContextHelper({
sidebarOpen = true, activeBlock = null, filterUnit = null, filteredBlocks = [],
activeBlock = null, filterUnit = null, filteredBlocks = [],
}: InitialState) {
const {
setSidebarOpen, setActiveBlock, setFilterUnit, setFilteredBlocks,
setActiveBlock, setFilterUnit, setFilteredBlocks,
} = useAspectsSidebarContext();

// This effect is run only one to set the initial state for the tests.
React.useEffect(() => {
setSidebarOpen(sidebarOpen);
}, []); // eslint-disable-line react-hooks/exhaustive-deps

// The sidebar will clear active/filter stuff to render cleanly on initalization
// So, we can't set these values in the useEffect above. The only way to cleanly
// do it is by userEvent.click - simulating real-world scenario
Expand All @@ -109,7 +118,7 @@ function TestContextHelper({

describe('AspectsSidebar', () => {
const renderComponent = (
initialState: InitialState = { sidebarOpen: true },
initialState: InitialState = {},
props?: Partial<React.ComponentProps<typeof AspectsSidebar>>,
) => render(

Expand All @@ -126,19 +135,6 @@ describe('AspectsSidebar', () => {
jest.clearAllMocks();
});

it('closes the sidebar when the close button is clicked', async () => {
renderComponent();
// Ensure the sidebar is initially open and visible
expect(screen.getByTestId('sidebar')).toBeVisible();
expect(screen.getByTestId('sidebar-title')).toHaveTextContent('Test Course');

const closeButton = screen.getByRole('button', { name: 'Close' });
fireEvent.click(closeButton);

// Assert that the sidebar is no longer visible
expect(screen.queryByTestId('sidebar')).not.toBeInTheDocument();
});

it('shows the back button and changes the title when a block is clicked', () => {
renderComponent();

Expand Down Expand Up @@ -168,7 +164,7 @@ describe('AspectsSidebar', () => {

it('shows an Alert message when the content lists are empty on a Unit Page', () => {
// NOTE: Currently there is not "Unit Page Dashboard", hence the alert
renderComponent({ sidebarOpen: true }, { blockType: BlockTypes.vertical, contentLists: [] });
renderComponent(undefined, { blockType: BlockTypes.vertical, contentLists: [] });

expect(mockDashboard).not.toHaveBeenCalled();
expect(screen.getByRole('alert')).toHaveTextContent(messages.noAnalyticsForUnit.defaultMessage);
Expand All @@ -177,7 +173,6 @@ describe('AspectsSidebar', () => {
it('shows filtered set of components in the Course Outline view when specific unit is selected', () => {
// render the component as if the "UnitActionsButton" has been clicked
renderComponent({
sidebarOpen: true,
activeBlock: mockUnit,
filterUnit: mockUnit,
filteredBlocks: ['block-v1:TEST+COURSE+SECTION1+prob2'],
Expand All @@ -194,7 +189,6 @@ describe('AspectsSidebar', () => {

it('navigate to component and back in filtered unit view', () => {
renderComponent({
sidebarOpen: true,
activeBlock: mockUnit,
filterUnit: mockUnit,
filteredBlocks: ['block-v1:TEST+COURSE+SECTION1+prob2'],
Expand All @@ -220,7 +214,7 @@ describe('AspectsSidebar', () => {

it('posts a callback with the activatedBlock in Unit Page View', () => {
const callback = jest.fn();
renderComponent({ sidebarOpen: true }, {
renderComponent(undefined, {
title: 'Test Unit',
blockType: BlockTypes.vertical,
contentLists: mockContentLists.slice(0, 1),
Expand Down
Loading