Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 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
2 changes: 1 addition & 1 deletion src/courseware/course/Course.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ const Course = ({
const SidebarProviderComponent = isNewDiscussionSidebarViewEnabled ? NewSidebarProvider : SidebarProvider;

return (
<SidebarProviderComponent courseId={courseId} unitId={unitId}>
<SidebarProviderComponent courseId={courseId} unitId={unitId} sectionId={section ? section.id : null}>
<Helmet>
<title>{`${pageTitleBreadCrumbs.join(' | ')} | ${getConfig().SITE_NAME}`}</title>
</Helmet>
Expand Down
7 changes: 4 additions & 3 deletions src/courseware/course/Course.test.jsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import React from 'react';

import { Factory } from 'rosie';

import { breakpoints } from '@openedx/paragon';
Expand All @@ -11,6 +9,7 @@ import * as celebrationUtils from './celebration/utils';
import { handleNextSectionCelebration } from './celebration';
import Course from './Course';
import setupDiscussionSidebar from './test-utils';
import messages from './messages';

jest.mock('@edx/frontend-platform/analytics');
jest.mock('@edx/frontend-lib-special-exams/dist/data/thunks.js', () => ({
Expand Down Expand Up @@ -194,7 +193,9 @@ describe('Course', () => {
it('handles click to open/close notification tray', async () => {
await setupDiscussionSidebar();
waitFor(() => {
const notificationShowButton = screen.findByRole('button', { name: /Show notification tray/i });
const notificationShowButton = screen.findByRole('button', {
name: messages.openNotificationTrigger.defaultMessage,
});
expect(screen.queryByRole('region', { name: /notification tray/i })).not.toBeInTheDocument();
fireEvent.click(notificationShowButton);
expect(screen.queryByRole('region', { name: /notification tray/i })).toBeInTheDocument();
Expand Down
1 change: 1 addition & 0 deletions src/courseware/course/JumpNavMenuItem.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const mockData = {
sectionId: 'block-v1:edX+DemoX+Demo_Course+type@chapter+block@interactive_demonstrations',
sequenceId: 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@basic_questions',

currentSequence: 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@basic_questions',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious. Why was this change only added now?

Copy link
Author

Choose a reason for hiding this comment

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

my mistake, removed

title: 'Demo Menu Item',
courseId: 'course-v1:edX+DemoX+Demo_Course',
currentUnit: 'block-v1:edX+DemoX+Demo_Course+type@chapter+block@interactive_demonstrations',
Expand Down
2 changes: 1 addition & 1 deletion src/courseware/course/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const messages = defineMessages({
},
openNotificationTrigger: {
id: 'notification.open.button',
defaultMessage: 'Show notification tray',
defaultMessage: 'Notifications tray',
description: 'Button to open the notification tray and show notifications',
},
closeNotificationTrigger: {
Expand Down
5 changes: 4 additions & 1 deletion src/courseware/course/sidebar/SidebarContextProvider.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { SIDEBARS } from './sidebars';
const SidebarProvider = ({
courseId,
unitId,
sectionId,
children,
}) => {
const { verifiedMode } = useModel('courseHomeMeta', courseId);
Expand Down Expand Up @@ -72,8 +73,9 @@ const SidebarProvider = ({
shouldDisplayFullScreen,
courseId,
unitId,
sectionId,
}), [courseId, currentSidebar, notificationStatus, onNotificationSeen, shouldDisplayFullScreen,
shouldDisplaySidebarOpen, toggleSidebar, unitId, upgradeNotificationCurrentState]);
shouldDisplaySidebarOpen, toggleSidebar, unitId, upgradeNotificationCurrentState, sectionId]);

return (
<SidebarContext.Provider value={contextValue}>
Expand All @@ -85,6 +87,7 @@ const SidebarProvider = ({
SidebarProvider.propTypes = {
courseId: PropTypes.string.isRequired,
unitId: PropTypes.string.isRequired,
sectionId: PropTypes.string.isRequired,
children: PropTypes.node,
};

Expand Down
152 changes: 152 additions & 0 deletions src/courseware/course/sidebar/common/Sidebar.test.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this React import?

Copy link
Author

Choose a reason for hiding this comment

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

removed

import { Factory } from 'rosie';
import {
Comment on lines 1 to 5
Copy link
Contributor

Choose a reason for hiding this comment

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

[code style]: Let’s separate external library imports from local imports with a blank line for better readability.

Suggested change
import { Factory } from 'rosie';
import {
import { Factory } from 'rosie';
import {

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the line import { createRef } from 'react'; come before import { Factory } from 'rosie';? The idea is to separate library imports from local imports.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

initializeTestStore,
render,
screen,
fireEvent,
waitFor,
} from '@src/setupTest';
import userEvent from '@testing-library/user-event';
import SidebarContext from '../SidebarContext';
import SidebarBase from './SidebarBase';
import messages from '../../messages';
import { useSidebarFocusAndKeyboard } from './hooks/useSidebarFocusAndKeyboard';

jest.mock('./hooks/useSidebarFocusAndKeyboard');

const SIDEBAR_ID = 'test-sidebar';

const mockUseSidebarFocusAndKeyboard = useSidebarFocusAndKeyboard;

describe('SidebarBase (Refactored)', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
describe('SidebarBase (Refactored)', () => {
describe('SidebarBase', () => {

Copy link
Author

Choose a reason for hiding this comment

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

fixed

let mockContextValue;
const courseMetadata = Factory.build('courseMetadata');
const user = userEvent.setup();

let mockCloseBtnRef;
let mockBackBtnRef;
let mockHandleClose;
let mockHandleKeyDown;
let mockHandleBackBtnKeyDown;

const renderSidebar = (contextProps = {}, componentProps = {}) => {
const fullContextValue = { ...mockContextValue, ...contextProps };
const defaultProps = {
title: 'Test Sidebar Title',
ariaLabel: 'Test Sidebar Aria Label',
sidebarId: SIDEBAR_ID,
className: 'test-class',
children: <div>Sidebar Content</div>,
...componentProps,
};
return render(
<SidebarContext.Provider value={fullContextValue}>
<SidebarBase {...defaultProps} />
</SidebarContext.Provider>,
);
};

beforeEach(async () => {
await initializeTestStore({
courseMetadata,
excludeFetchCourse: true,
excludeFetchSequence: true,
});

mockContextValue = {
courseId: courseMetadata.id,
toggleSidebar: jest.fn(),
shouldDisplayFullScreen: false,
currentSidebar: null,
};

mockCloseBtnRef = React.createRef();
mockBackBtnRef = React.createRef();
mockHandleClose = jest.fn();
mockHandleKeyDown = jest.fn();
mockHandleBackBtnKeyDown = jest.fn();

mockUseSidebarFocusAndKeyboard.mockReturnValue({
closeBtnRef: mockCloseBtnRef,
backBtnRef: mockBackBtnRef,
handleClose: mockHandleClose,
handleKeyDown: mockHandleKeyDown,
handleBackBtnKeyDown: mockHandleBackBtnKeyDown,
});
});

afterEach(() => {
jest.clearAllMocks();
});

it('should render children, title, and close button when visible', () => {
renderSidebar({ currentSidebar: SIDEBAR_ID });
expect(screen.getByText('Sidebar Content')).toBeInTheDocument();
expect(screen.getByText('Test Sidebar Title')).toBeInTheDocument();
expect(screen.getByRole('button', { name: messages.closeNotificationTrigger.defaultMessage })).toBeInTheDocument();
expect(screen.getByTestId(`sidebar-${SIDEBAR_ID}`)).toBeInTheDocument();
expect(screen.getByTestId(`sidebar-${SIDEBAR_ID}`)).not.toHaveClass('d-none');
});

it('should be hidden via CSS class when not the current sidebar', () => {
renderSidebar({ currentSidebar: 'another-sidebar-id' });
const sidebarElement = screen.queryByTestId(`sidebar-${SIDEBAR_ID}`);
expect(sidebarElement).toBeInTheDocument();
expect(sidebarElement).toHaveClass('d-none');
});

it('should hide title bar when showTitleBar prop is false', () => {
renderSidebar({ currentSidebar: SIDEBAR_ID }, { showTitleBar: false });
expect(screen.queryByText('Test Sidebar Title')).not.toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we replace the static text in these tests with values ​​from defaultProps?

Copy link
Author

Choose a reason for hiding this comment

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

fixed

expect(screen.queryByRole('button', { name: messages.closeNotificationTrigger.defaultMessage })).not.toBeInTheDocument();
expect(screen.getByText('Sidebar Content')).toBeInTheDocument();
});

it('should render back button instead of close button in fullscreen', () => {
renderSidebar({ currentSidebar: SIDEBAR_ID, shouldDisplayFullScreen: true });
expect(screen.getByRole('button', { name: messages.responsiveCloseNotificationTray.defaultMessage })).toBeInTheDocument();
expect(screen.queryByRole('button', { name: messages.closeNotificationTrigger.defaultMessage })).not.toBeInTheDocument();
});

it('should call handleClose from hook on close button click', async () => {
renderSidebar({ currentSidebar: SIDEBAR_ID });
const closeButton = screen.getByRole('button', { name: messages.closeNotificationTrigger.defaultMessage });
await user.click(closeButton);
expect(mockHandleClose).toHaveBeenCalledTimes(1);
});

it('should call handleClose from hook on fullscreen back button click', async () => {
renderSidebar({ currentSidebar: SIDEBAR_ID, shouldDisplayFullScreen: true });
const backButton = screen.getByRole('button', { name: messages.responsiveCloseNotificationTray.defaultMessage });
await user.click(backButton);
expect(mockHandleClose).toHaveBeenCalledTimes(1);
});

it('should call handleKeyDown from hook on standard close button keydown', () => {

Check failure on line 126 in src/courseware/course/sidebar/common/Sidebar.test.jsx

View workflow job for this annotation

GitHub Actions / tests

Expected indentation of 2 spaces but found 3
renderSidebar({ currentSidebar: SIDEBAR_ID });

Check failure on line 127 in src/courseware/course/sidebar/common/Sidebar.test.jsx

View workflow job for this annotation

GitHub Actions / tests

Expected indentation of 4 spaces but found 5
const closeButton = screen.getByRole('button', { name: messages.closeNotificationTrigger.defaultMessage });

Check failure on line 128 in src/courseware/course/sidebar/common/Sidebar.test.jsx

View workflow job for this annotation

GitHub Actions / tests

Expected indentation of 4 spaces but found 5
fireEvent.keyDown(closeButton, { key: 'Tab' });

Check failure on line 129 in src/courseware/course/sidebar/common/Sidebar.test.jsx

View workflow job for this annotation

GitHub Actions / tests

Expected indentation of 4 spaces but found 5
expect(mockHandleKeyDown).toHaveBeenCalledTimes(1);

Check failure on line 130 in src/courseware/course/sidebar/common/Sidebar.test.jsx

View workflow job for this annotation

GitHub Actions / tests

Expected indentation of 4 spaces but found 5
});

Check failure on line 131 in src/courseware/course/sidebar/common/Sidebar.test.jsx

View workflow job for this annotation

GitHub Actions / tests

Expected indentation of 2 spaces but found 3

it('should call handleBackBtnKeyDown from hook on fullscreen back button keydown', () => {

Check failure on line 133 in src/courseware/course/sidebar/common/Sidebar.test.jsx

View workflow job for this annotation

GitHub Actions / tests

Expected indentation of 2 spaces but found 3
renderSidebar({ currentSidebar: SIDEBAR_ID, shouldDisplayFullScreen: true });

Check failure on line 134 in src/courseware/course/sidebar/common/Sidebar.test.jsx

View workflow job for this annotation

GitHub Actions / tests

Expected indentation of 4 spaces but found 5
const backButton = screen.getByRole('button', { name: messages.responsiveCloseNotificationTray.defaultMessage });

Check failure on line 135 in src/courseware/course/sidebar/common/Sidebar.test.jsx

View workflow job for this annotation

GitHub Actions / tests

Expected indentation of 4 spaces but found 5
fireEvent.keyDown(backButton, { key: 'Enter' });

Check failure on line 136 in src/courseware/course/sidebar/common/Sidebar.test.jsx

View workflow job for this annotation

GitHub Actions / tests

Expected indentation of 4 spaces but found 5
expect(mockHandleBackBtnKeyDown).toHaveBeenCalledTimes(1);
});

it('should call toggleSidebar(null) upon receiving a "close" postMessage event', async () => {
renderSidebar({ currentSidebar: SIDEBAR_ID });

fireEvent(window, new MessageEvent('message', {
data: { type: 'learning.events.sidebar.close' },
}));

await waitFor(() => {
expect(mockContextValue.toggleSidebar).toHaveBeenCalledTimes(1);
expect(mockContextValue.toggleSidebar).toHaveBeenCalledWith(null);
});
});
});
32 changes: 25 additions & 7 deletions src/courseware/course/sidebar/common/SidebarBase.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@ import { Icon, IconButton } from '@openedx/paragon';
import { ArrowBackIos, Close } from '@openedx/paragon/icons';
import classNames from 'classnames';
import PropTypes from 'prop-types';
import { useCallback, useContext } from 'react';
import {
useCallback, useContext,
} from 'react';

import { useEventListener } from '@src/generic/hooks';
import messages from '../../messages';
import SidebarContext from '../SidebarContext';
import { useSidebarFocusAndKeyboard } from './hooks/useSidebarFocusAndKeyboard';

const SidebarBase = ({
title,
Expand All @@ -24,13 +28,23 @@ const SidebarBase = ({
currentSidebar,
} = useContext(SidebarContext);

const {
closeBtnRef,
backBtnRef,
handleClose,
handleKeyDown,
handleBackBtnKeyDown,
} = useSidebarFocusAndKeyboard(sidebarId);

const isOpen = currentSidebar === sidebarId;

const receiveMessage = useCallback(({ data }) => {
const { type } = data;
if (type === 'learning.events.sidebar.close') {
toggleSidebar(null);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [sidebarId, toggleSidebar]);
}, [toggleSidebar]);
Copy link
Contributor

Choose a reason for hiding this comment

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

[clarify]: Is there any reason why sidebarId was removed from deps? Also, do I need to remove // ​​eslint-disable-next-line react-hooks/exhaustive-deps?

Copy link
Author

Choose a reason for hiding this comment

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

no reason, removed

Copy link
Contributor

Choose a reason for hiding this comment

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

My question is related to the fact that sidebarId was previously included in the dependency array. Why was it removed?

Copy link
Author

Choose a reason for hiding this comment

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

because sidebarId it was an unnecessary dependency


useEventListener('message', receiveMessage);

Expand All @@ -39,7 +53,7 @@ const SidebarBase = ({
className={classNames('ml-0 border border-light-400 rounded-sm h-auto align-top zindex-0', {
'bg-white m-0 border-0 fixed-top vh-100 rounded-0': shouldDisplayFullScreen,
'align-self-start': !shouldDisplayFullScreen,
'd-none': currentSidebar !== sidebarId,
'd-none': !isOpen,
}, className)}
data-testid={`sidebar-${sidebarId}`}
style={{ width: shouldDisplayFullScreen ? '100%' : width }}
Expand All @@ -49,9 +63,10 @@ const SidebarBase = ({
{shouldDisplayFullScreen ? (
<div
className="pt-2 pb-2.5 border-bottom border-light-400 d-flex align-items-center ml-2"
onClick={() => toggleSidebar(null)}
onKeyDown={() => toggleSidebar(null)}
onClick={handleClose}
onKeyDown={handleBackBtnKeyDown}
role="button"
ref={backBtnRef}
tabIndex="0"
>
<Icon src={ArrowBackIos} />
Expand All @@ -63,16 +78,19 @@ const SidebarBase = ({
{showTitleBar && (
<>
<div className="d-flex align-items-center mb-2">
<strong className="p-2.5 d-inline-block course-sidebar-title">{title}</strong>
<h2 className="p-2.5 d-inline-block m-0 text-gray-700 h4">{title}</h2>
{shouldDisplayFullScreen
? null
: (
<div className="d-inline-flex mr-2 ml-auto">
<IconButton
className="sidebar-close-btn"
src={Close}
size="sm"
ref={closeBtnRef}
iconAs={Icon}
onClick={() => toggleSidebar(null)}
onClick={handleClose}
onKeyDown={handleKeyDown}
variant="primary"
alt={intl.formatMessage(messages.closeNotificationTrigger)}
/>
Expand Down
20 changes: 15 additions & 5 deletions src/courseware/course/sidebar/common/TriggerBase.jsx
Original file line number Diff line number Diff line change
@@ -1,27 +1,37 @@
import PropTypes from 'prop-types';
import React from 'react';
import { forwardRef } from 'react';

const SidebarTriggerBase = ({
const SidebarTriggerBase = forwardRef(({
onClick,
onKeyDown,
ariaLabel,
children,
}) => (
isOpenNotificationStatusBar,
sectionId,
}, ref) => (
<button
className="border border-light-400 bg-transparent align-items-center align-content-center d-flex notification-btn"
className="border border-light-400 bg-transparent align-items-center align-content-center d-flex notification-btn sidebar-trigger-btn"
type="button"
onClick={onClick}
onKeyDown={onKeyDown}
aria-label={ariaLabel}
aria-expanded={isOpenNotificationStatusBar}
aria-controls={sectionId}
ref={ref}
>
<div className="icon-container d-flex position-relative align-items-center">
{children}
</div>
</button>
);
));

SidebarTriggerBase.propTypes = {
onClick: PropTypes.func.isRequired,
onKeyDown: PropTypes.func.isRequired,
ariaLabel: PropTypes.string.isRequired,
children: PropTypes.element.isRequired,
isOpenNotificationStatusBar: PropTypes.bool.isRequired,
sectionId: PropTypes.string.isRequired,
};

export default SidebarTriggerBase;
Loading