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.

Sign up for GitHub

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

Jump to bottom

[FC-0056][Plugin] Course outline sidebar (plugin wrapper) #1349

Closed
wants to merge 2 commits into from
Closed

[FC-0056][Plugin] Course outline sidebar (plugin wrapper) #1349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b70509f
feat: [FC-0056] create course outline sidebar
ihor-romaniuk Mar 11, 2024
219ecb8
feat: [FC-0056] cover course outline sidebar into plugin slot
ihor-romaniuk Apr 9, 2024
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
13 changes: 13 additions & 0 deletions example.env.config.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import UnitTranslationPlugin from '@edx/unit-translation-selector-plugin';
import { PLUGIN_OPERATIONS, DIRECT_PLUGIN } from '@openedx/frontend-plugin-framework';

import {
OUTLINE_SIDEBAR_DESKTOP_PLUGIN_SLOT_ID,
OUTLINE_SIDEBAR_MOBILE_PLUGIN_SLOT_ID,
} from '@src/courseware/course/sidebar/sidebars/course-outline';

// Load environment variables from .env file
const config = {
...process.env,
Expand All @@ -18,6 +23,14 @@ const config = {
},
],
},
[OUTLINE_SIDEBAR_DESKTOP_PLUGIN_SLOT_ID]: {
keepDefault: true,
plugins: [],
},
[OUTLINE_SIDEBAR_MOBILE_PLUGIN_SLOT_ID]: {
keepDefault: true,
plugins: [],
},
},
};

Expand Down
12 changes: 12 additions & 0 deletions src/course-home/data/__snapshots__/redux.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ Object {
},
"courseware": Object {
"courseId": null,
"courseOutline": Object {},
"courseOutlineShouldUpdate": false,
"courseOutlineStatus": "loading",
"courseStatus": "loading",
"rightSidebarSettings": Object {},
"sequenceId": null,
"sequenceMightBeUnit": false,
"sequenceStatus": "loading",
Expand Down Expand Up @@ -401,7 +405,11 @@ Object {
},
"courseware": Object {
"courseId": null,
"courseOutline": Object {},
"courseOutlineShouldUpdate": false,
"courseOutlineStatus": "loading",
"courseStatus": "loading",
"rightSidebarSettings": Object {},
"sequenceId": null,
"sequenceMightBeUnit": false,
"sequenceStatus": "loading",
Expand Down Expand Up @@ -669,7 +677,11 @@ Object {
},
"courseware": Object {
"courseId": null,
"courseOutline": Object {},
"courseOutlineShouldUpdate": false,
"courseOutlineStatus": "loading",
"courseStatus": "loading",
"rightSidebarSettings": Object {},
"sequenceId": null,
"sequenceMightBeUnit": false,
"sequenceStatus": "loading",
Expand Down
19 changes: 9 additions & 10 deletions src/course-home/outline-tab/SequenceLink.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import React from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import { Link } from 'react-router-dom';
Expand Down Expand Up @@ -96,15 +95,15 @@ const SequenceLink = ({
icon={fasCheckCircle}
fixedWidth
className="float-left text-success mt-1"
aria-hidden="true"
aria-hidden={complete}
title={intl.formatMessage(messages.completedAssignment)}
/>
) : (
<FontAwesomeIcon
icon={farCheckCircle}
fixedWidth
className="float-left text-gray-400 mt-1"
aria-hidden="true"
aria-hidden={complete}
title={intl.formatMessage(messages.incompleteAssignment)}
/>
)}
Expand All @@ -118,14 +117,14 @@ const SequenceLink = ({
</div>
</div>
{hideFromTOC && (
<div className="row w-100 my-2 mx-4 pl-3">
<span className="small d-flex">
<Icon className="mr-2" src={Block} data-testid="hide-from-toc-sequence-link-icon" />
<span data-testid="hide-from-toc-sequence-link-text">
{intl.formatMessage(messages.hiddenSequenceLink)}
<div className="row w-100 my-2 mx-4 pl-3">
<span className="small d-flex">
<Icon className="mr-2" src={Block} data-testid="hide-from-toc-sequence-link-icon" />
<span data-testid="hide-from-toc-sequence-link-text">
{intl.formatMessage(messages.hiddenSequenceLink)}
</span>
</span>
</span>
</div>
</div>
)}
<div className="row w-100 m-0 ml-3 pl-3">
<small className="text-body pl-2">
Expand Down
41 changes: 22 additions & 19 deletions src/courseware/course/Course.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,27 @@
import React, { useEffect, useState } from 'react';
import { useEffect, useState } from 'react';
import PropTypes from 'prop-types';
import { Helmet } from 'react-helmet';
import { useDispatch } from 'react-redux';
import { getConfig } from '@edx/frontend-platform';
import { breakpoints, useWindowSize } from '@openedx/paragon';

import { AlertList } from '../../generic/user-messages';

import Sequence from './sequence';

import { CelebrationModal, shouldCelebrateOnSectionLoad, WeeklyGoalCelebrationModal } from './celebration';
import { PluginSlot } from '@openedx/frontend-plugin-framework';

import { AlertList } from '@src/generic/user-messages';
import { useModel } from '@src/generic/model-store';
import {
OUTLINE_SIDEBAR_DESKTOP_PLUGIN_SLOT_ID,
Trigger as CourseOutlineTrigger,
checkIsOutlineSidebarAvailable,
} from './sidebar/sidebars/course-outline';
import Chat from './chat/Chat';
import ContentTools from './content-tools';
import CourseBreadcrumbs from './CourseBreadcrumbs';
import SidebarProvider from './sidebar/SidebarContextProvider';
import SidebarTriggers from './sidebar/SidebarTriggers';
import NewSidebarProvider from './new-sidebar/SidebarContextProvider';
import NewSidebarTriggers from './new-sidebar/SidebarTriggers';

import { useModel } from '../../generic/model-store';
import { CelebrationModal, shouldCelebrateOnSectionLoad, WeeklyGoalCelebrationModal } from './celebration';
import CourseBreadcrumbs from './CourseBreadcrumbs';
import ContentTools from './content-tools';
import Sequence from './sequence';

const Course = ({
courseId,
Expand All @@ -37,7 +40,7 @@ const Course = ({
const sequence = useModel('sequences', sequenceId);
const section = useModel('sections', sequence ? sequence.sectionId : null);
const enableNewSidebar = getConfig().ENABLE_NEW_SIDEBAR;
const navigationDisabled = sequence?.navigationDisabled ?? false;
Copy link

Choose a reason for hiding this comment

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

Can we get rid of this env var and just enable it by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if we should include refactoring the existing sidebars and removing the old one.
If we want to keep only the new sidebar, then we need to delete all links and files regarding the old sidebar, which can take a lot of time.

const navigationDisabled = checkIsOutlineSidebarAvailable() || (sequence?.navigationDisabled ?? false);

const pageTitleBreadCrumbs = [
sequence,
Expand All @@ -54,7 +57,6 @@ const Course = ({
const [weeklyGoalCelebrationOpen, setWeeklyGoalCelebrationOpen] = useState(
celebrations && !celebrations.streakLengthToCelebrate && celebrations.weeklyGoal,
);
const shouldDisplayTriggers = windowWidth >= breakpoints.small.minWidth;
const shouldDisplayChat = windowWidth >= breakpoints.medium.minWidth;
const daysPerWeek = course?.courseGoals?.selectedGoal?.daysPerWeek;

Expand All @@ -76,7 +78,7 @@ const Course = ({
<Helmet>
<title>{`${pageTitleBreadCrumbs.join(' | ')} | ${getConfig().SITE_NAME}`}</title>
</Helmet>
<div className="position-relative d-flex align-items-center mb-4 mt-1">
<div className="position-relative d-flex align-items-xl-center mb-4 mt-1 flex-column flex-xl-row">
{navigationDisabled || (
<>
<CourseBreadcrumbs
Expand All @@ -100,11 +102,12 @@ const Course = ({
/>
</>
)}
{shouldDisplayTriggers && (
<>
{enableNewSidebar === 'true' ? <NewSidebarTriggers /> : <SidebarTriggers /> }
</>
)}
<div className="w-100 d-flex align-items-center">
<PluginSlot id={OUTLINE_SIDEBAR_DESKTOP_PLUGIN_SLOT_ID}>
<CourseOutlineTrigger isMobileView />
</PluginSlot>
{enableNewSidebar === 'true' ? <NewSidebarTriggers /> : <SidebarTriggers /> }
</div>
Copy link

Choose a reason for hiding this comment

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

Again, it would be best if we just went with it and removed the old triggers.

Copy link
Contributor Author

@ihor-romaniuk ihor-romaniuk Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This all involves refactoring the new and old sidebars, which I wouldn't include in this PR.

</div>

<AlertList topic="sequence" />
Expand Down
34 changes: 20 additions & 14 deletions src/courseware/course/Course.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Factory } from 'rosie';
import { breakpoints } from '@openedx/paragon';

import {
act, fireEvent, getByRole, initializeTestStore, loadUnit, render, screen, waitFor,
fireEvent, getByRole, initializeTestStore, loadUnit, render, screen, waitFor,
} from '../../setupTest';
import * as celebrationUtils from './celebration/utils';
import { handleNextSectionCelebration } from './celebration';
Expand Down Expand Up @@ -59,7 +59,7 @@ describe('Course', () => {

it('loads learning sequence', async () => {
render(<Course {...mockData} />, { wrapWithRouter: true });
expect(screen.getByRole('navigation', { name: 'breadcrumb' })).toBeInTheDocument();
expect(screen.queryByRole('navigation', { name: 'breadcrumb' })).toBeInTheDocument();
expect(await screen.findByText('Loading learning sequence...')).toBeInTheDocument();

expect(screen.queryByRole('alert')).not.toBeInTheDocument();
Expand Down Expand Up @@ -142,27 +142,32 @@ describe('Course', () => {

const notificationTrigger = screen.getByRole('button', { name: /Show notification tray/i });
expect(notificationTrigger).toBeInTheDocument();
expect(notificationTrigger.parentNode).not.toHaveClass('mt-3', { exact: true });
expect(notificationTrigger.parentNode).not.toHaveClass('sidebar-active', { exact: true });
fireEvent.click(notificationTrigger);
expect(notificationTrigger.parentNode).toHaveClass('mt-3');
expect(notificationTrigger.parentNode).toHaveClass('sidebar-active');
});

it('handles click to open/close discussions sidebar', async () => {
await setupDiscussionSidebar();
const discussionsTrigger = await screen.getByRole('button', { name: /Show discussions tray/i });
const discussionsSideBar = await waitFor(() => screen.findByTestId('sidebar-DISCUSSIONS'));

expect(discussionsSideBar).not.toHaveClass('d-none');
await waitFor(() => {
expect(screen.getByTestId('sidebar-DISCUSSIONS')).toBeInTheDocument();
expect(screen.getByTestId('sidebar-DISCUSSIONS')).not.toHaveClass('d-none');
});

const discussionsTrigger = await screen.getByRole('button', { name: /Show discussions tray/i });
expect(discussionsTrigger).toBeInTheDocument();
fireEvent.click(discussionsTrigger);

await act(async () => {
fireEvent.click(discussionsTrigger);
await waitFor(() => {
expect(screen.queryByTestId('sidebar-DISCUSSIONS')).not.toBeInTheDocument();
});
await expect(discussionsSideBar).toHaveClass('d-none');

await act(async () => {
fireEvent.click(discussionsTrigger);
fireEvent.click(discussionsTrigger);

await waitFor(() => {
expect(screen.queryByTestId('sidebar-DISCUSSIONS')).toBeInTheDocument();
});
await expect(discussionsSideBar).not.toHaveClass('d-none');
});

it('displays discussions sidebar when unit changes', async () => {
Expand Down Expand Up @@ -192,8 +197,9 @@ describe('Course', () => {
it('handles click to open/close notification tray', async () => {
await setupDiscussionSidebar();
const notificationShowButton = await screen.findByRole('button', { name: /Show notification tray/i });
expect(screen.queryByRole('region', { name: /notification tray/i })).toHaveClass('d-none');
expect(screen.queryByRole('region', { name: /notification tray/i })).not.toBeInTheDocument();
fireEvent.click(notificationShowButton);
expect(screen.queryByRole('region', { name: /notification tray/i })).toBeInTheDocument();
expect(screen.queryByRole('region', { name: /notification tray/i })).not.toHaveClass('d-none');
});

Expand Down
2 changes: 1 addition & 1 deletion src/courseware/course/CourseBreadcrumbs.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ const CourseBreadcrumbs = ({
}, [courseStatus, sequenceStatus, allSequencesInSections]);

return (
<nav aria-label="breadcrumb" className="d-inline-block col-sm-10">
<nav aria-label="breadcrumb" className="d-inline-block col-sm-10 mb-3">
<ol className="list-unstyled d-flex flex-nowrap align-items-center m-0">
<li className="list-unstyled col-auto m-0 p-0">
<Link
Expand Down
Loading
Loading