feat: Update previous and next unit navigation buttons design (#1617)

* feat: Update previous and next unit navigation buttons design

* feat: add unit test

* feat: move unit navigation to be inline with unit title

Author: Rodra

src/courseware/course/sequence/Sequence.jsx

@@ -203,6 +203,8 @@ const Sequence = ({
               unitId={unitId}
               unitLoadedHandler={handleUnitLoaded}
               isOriginalUserStaff={originalUserIsStaff}
+              isEnabledOutlineSidebar={isEnabledOutlineSidebar}
+              renderUnitNavigation={renderUnitNavigation}
             />
             {unitHasLoaded && renderUnitNavigation(false)}
           </div>
@@ -223,7 +225,6 @@ const Sequence = ({
           originalUserIsStaff={originalUserIsStaff}
           canAccessProctoredExams={canAccessProctoredExams}
         >
-          {isEnabledOutlineSidebar && renderUnitNavigation(true)}
           {defaultContent}
         </SequenceExamWrapper>
         <CourseLicense license={license || undefined} />

src/courseware/course/sequence/SequenceContent.jsx

@@ -16,6 +16,8 @@ const SequenceContent = ({
   unitId,
   unitLoadedHandler,
   isOriginalUserStaff,
+  isEnabledOutlineSidebar,
+  renderUnitNavigation,
 }) => {
   const intl = useIntl();
   const sequence = useModel('sequences', sequenceId);
@@ -61,6 +63,8 @@ const SequenceContent = ({
       id={unitId}
       onLoaded={unitLoadedHandler}
       isOriginalUserStaff={isOriginalUserStaff}
+      isEnabledOutlineSidebar={isEnabledOutlineSidebar}
+      renderUnitNavigation={renderUnitNavigation}
     />
   );
 };
@@ -72,6 +76,8 @@ SequenceContent.propTypes = {
   unitId: PropTypes.string,
   unitLoadedHandler: PropTypes.func.isRequired,
   isOriginalUserStaff: PropTypes.bool.isRequired,
+  isEnabledOutlineSidebar: PropTypes.bool.isRequired,
+  renderUnitNavigation: PropTypes.func.isRequired,
 };
 
 SequenceContent.defaultProps = {

src/courseware/course/sequence/Unit/__snapshots__/index.test.jsx.snap

@@ -21,18 +21,22 @@ exports[`Unit component output snapshot: not bookmarked, do not show content 1`]
   className="unit"
 >
   <div
-    className="mb-0"
+    className="d-flex justify-content-between align-items-center"
   >
-    <h3
-      className="h3"
+    <div
+      className="mb-0"
     >
-      unit-title
-    </h3>
-    <UnitTitleSlot
-      courseId="test-course-id"
-      unitId="test-props-id"
-      unitTitle="unit-title"
-    />
+      <h3
+        className="h3"
+      >
+        unit-title
+      </h3>
+      <UnitTitleSlot
+        courseId="test-course-id"
+        unitId="test-props-id"
+        unitTitle="unit-title"
+      />
+    </div>
   </div>
   <p
     className="sr-only"

src/courseware/course/sequence/Unit/index.jsx

@@ -23,6 +23,8 @@ const Unit = ({
   onLoaded,
   id,
   isOriginalUserStaff,
+  isEnabledOutlineSidebar,
+  renderUnitNavigation,
 }) => {
   const { formatMessage } = useIntl();
   const [searchParams] = useSearchParams();
@@ -48,9 +50,12 @@ const Unit = ({
 
   return (
     <div className="unit">
-      <div className="mb-0">
-        <h3 className="h3">{unit.title}</h3>
-        <UnitTitleSlot courseId={courseId} unitId={id} unitTitle={unit.title} />
+      <div className="d-flex justify-content-between align-items-center">
+        <div className="mb-0">
+          <h3 className="h3">{unit.title}</h3>
+          <UnitTitleSlot courseId={courseId} unitId={id} unitTitle={unit.title} />
+        </div>
+        {isEnabledOutlineSidebar && renderUnitNavigation(true)}
       </div>
       <p className="sr-only">{formatMessage(messages.headerPlaceholder)}</p>
       <BookmarkButton
@@ -79,6 +84,8 @@ Unit.propTypes = {
   id: PropTypes.string.isRequired,
   onLoaded: PropTypes.func,
   isOriginalUserStaff: PropTypes.bool.isRequired,
+  isEnabledOutlineSidebar: PropTypes.bool.isRequired,
+  renderUnitNavigation: PropTypes.func.isRequired,
 };
 
 Unit.defaultProps = {

src/courseware/course/sequence/sequence-navigation/SequenceNavigation.jsx

@@ -88,7 +88,6 @@ const SequenceNavigation = ({
     return navigationDisabledNextSequence || (
       <NextUnitTopNavTriggerSlot
         {...{
-          courseId,
           disabled,
           buttonText,
           nextLink,

src/courseware/course/sequence/sequence-navigation/UnitNavigation.jsx

@@ -23,36 +23,40 @@ const UnitNavigation = ({
     isFirstUnit, isLastUnit, nextLink, previousLink,
   } = useSequenceNavigationMetadata(sequenceId, unitId);
 
-  const renderPreviousButton = () => (
-    <PreviousButton
-      isFirstUnit={isFirstUnit}
-      variant="outline-secondary"
-      buttonLabel={intl.formatMessage(messages.previousButton)}
-      buttonStyle="previous-button justify-content-center"
-      onClick={onClickPrevious}
-      previousLink={previousLink}
-    />
-  );
+  const renderPreviousButton = () => {
+    const buttonStyle = `previous-button ${isAtTop ? 'text-dark mr-3' : 'justify-content-center'}`;
+    return (
+      <PreviousButton
+        isFirstUnit={isFirstUnit}
+        variant="outline-secondary"
+        buttonLabel={intl.formatMessage(messages.previousButton)}
+        buttonStyle={buttonStyle}
+        onClick={onClickPrevious}
+        previousLink={previousLink}
+        isAtTop={isAtTop}
+      />
+    );
+  };
 
   const renderNextButton = () => {
     const { exitActive, exitText } = GetCourseExitNavigation(courseId, intl);
     const buttonText = (isLastUnit && exitText) ? exitText : intl.formatMessage(messages.nextButton);
     const disabled = isLastUnit && !exitActive;
     const variant = 'outline-primary';
-    const buttonStyle = 'next-button justify-content-center';
+    const buttonStyle = `next-button ${isAtTop ? 'text-dark' : 'justify-content-center'}`;
 
     if (isAtTop) {
       return (
         <NextUnitTopNavTriggerSlot
           {...{
-            courseId,
             variant,
             buttonStyle,
             buttonText,
             disabled,
             sequenceId,
             nextLink,
             onClickHandler: onClickNext,
+            isAtTop,
           }}
         />
       );
@@ -72,7 +76,11 @@ const UnitNavigation = ({
   };
 
   return (
-    <div className={classNames('unit-navigation d-flex', { 'top-unit-navigation mb-3 w-100': isAtTop })}>
+    <div className={classNames('d-flex', {
+      'unit-navigation': !isAtTop,
+      'top-unit-navigation': isAtTop,
+    })}
+    >
       {renderPreviousButton()}
       {renderNextButton()}
     </div>

src/courseware/course/sequence/sequence-navigation/UnitNavigation.test.jsx

@@ -5,6 +5,13 @@ import {
 } from '../../../../setupTest';
 import UnitNavigation from './UnitNavigation';
 
+const mockNavigate = jest.fn();
+
+jest.mock('react-router-dom', () => ({
+  ...jest.requireActual('react-router-dom'),
+  useNavigate: () => mockNavigate,
+}));
+
 describe('Unit Navigation', () => {
   let mockData;
   const courseMetadata = Factory.build('courseMetadata');
@@ -56,6 +63,26 @@ describe('Unit Navigation', () => {
     expect(onClickNext).toHaveBeenCalledTimes(1);
   });
 
+  it('when clicked it calls navigate when is at the top', () => {
+    const onClickPrevious = jest.fn();
+    const onClickNext = jest.fn();
+
+    render(<UnitNavigation
+      {...mockData}
+      onClickPrevious={onClickPrevious}
+      onClickNext={onClickNext}
+      isAtTop
+    />, { wrapWithRouter: true });
+
+    fireEvent.click(screen.getByRole('button', { name: /previous/i }));
+    expect(onClickPrevious).toHaveBeenCalledTimes(1);
+    expect(mockNavigate).toHaveBeenCalledTimes(1);
+
+    fireEvent.click(screen.getByRole('button', { name: /next/i }));
+    expect(onClickNext).toHaveBeenCalledTimes(1);
+    expect(mockNavigate).toHaveBeenCalledTimes(2);
+  });
+
   it('has the navigation buttons enabled for the non-corner unit in the sequence', () => {
     render(<UnitNavigation {...mockData} />, { wrapWithRouter: true });
 

src/courseware/course/sequence/sequence-navigation/generic/NextButton.jsx

@@ -1,7 +1,12 @@
 import PropTypes from 'prop-types';
-import { Link, useLocation } from 'react-router-dom';
-import { Button } from '@openedx/paragon';
-import { ChevronLeft, ChevronRight } from '@openedx/paragon/icons';
+import { Link, useLocation, useNavigate } from 'react-router-dom';
+import { Button, IconButton, Icon } from '@openedx/paragon';
+import {
+  ArrowBack,
+  ArrowForward,
+  ChevronLeft,
+  ChevronRight,
+} from '@openedx/paragon/icons';
 import { isRtl, getLocale } from '@edx/frontend-platform/i18n';
 
 import UnitNavigationEffortEstimate from '../UnitNavigationEffortEstimate';
@@ -14,8 +19,9 @@ const NextButton = ({
   buttonStyle,
   disabled,
   hasEffortEstimate,
+  isAtTop,
 }) => {
-  const nextArrow = isRtl(getLocale()) ? ChevronLeft : ChevronRight;
+  const navigate = useNavigate();
   const { pathname } = useLocation();
   const navLink = pathname.startsWith('/preview') ? `/preview${nextLink}` : nextLink;
   const buttonContent = hasEffortEstimate ? (
@@ -24,6 +30,34 @@ const NextButton = ({
     </UnitNavigationEffortEstimate>
   ) : buttonText;
 
+  const getNextArrow = () => {
+    if (isAtTop) {
+      return isRtl(getLocale()) ? ArrowBack : ArrowForward;
+    }
+    return isRtl(getLocale()) ? ChevronLeft : ChevronRight;
+  };
+
+  const nextArrow = getNextArrow();
+
+  const onClick = () => {
+    navigate(navLink);
+    onClickHandler();
+  };
+
+  if (isAtTop) {
+    return (
+      <IconButton
+        variant="light"
+        className={buttonStyle}
+        onClick={onClick}
+        src={nextArrow}
+        disabled={disabled}
+        iconAs={Icon}
+        alt={buttonText}
+      />
+    );
+  }
+
   return (
     <Button
       variant={variant}
@@ -51,6 +85,7 @@ NextButton.propTypes = {
   buttonStyle: PropTypes.string.isRequired,
   disabled: PropTypes.bool.isRequired,
   hasEffortEstimate: PropTypes.bool,
+  isAtTop: PropTypes.bool.isRequired,
 };
 
 export default NextButton;

src/courseware/course/sequence/sequence-navigation/generic/PreviousButton.jsx

@@ -1,7 +1,12 @@
 import PropTypes from 'prop-types';
-import { Link, useLocation } from 'react-router-dom';
-import { Button } from '@openedx/paragon';
-import { ChevronLeft, ChevronRight } from '@openedx/paragon/icons';
+import { Link, useLocation, useNavigate } from 'react-router-dom';
+import { Button, IconButton, Icon } from '@openedx/paragon';
+import {
+  ArrowBack,
+  ArrowForward,
+  ChevronLeft,
+  ChevronRight,
+} from '@openedx/paragon/icons';
 import { isRtl, getLocale } from '@edx/frontend-platform/i18n';
 
 const PreviousButton = ({
@@ -11,12 +16,41 @@ const PreviousButton = ({
   variant,
   buttonStyle,
   isFirstUnit,
+  isAtTop,
 }) => {
+  const navigate = useNavigate();
   const disabled = isFirstUnit;
-  const prevArrow = isRtl(getLocale()) ? ChevronRight : ChevronLeft;
   const { pathname } = useLocation();
   const navLink = pathname.startsWith('/preview') ? `/preview${previousLink}` : previousLink;
 
+  const getPrevArrow = () => {
+    if (isAtTop) {
+      return isRtl(getLocale()) ? ArrowForward : ArrowBack;
+    }
+    return isRtl(getLocale()) ? ChevronRight : ChevronLeft;
+  };
+
+  const prevArrow = getPrevArrow();
+
+  const onClickHandler = () => {
+    navigate(navLink);
+    onClick();
+  };
+
+  if (isAtTop) {
+    return (
+      <IconButton
+        variant="light"
+        className={buttonStyle}
+        onClick={onClickHandler}
+        src={prevArrow}
+        disabled={disabled}
+        iconAs={Icon}
+        alt={buttonLabel}
+      />
+    );
+  }
+
   return (
     <Button
       variant={variant}
@@ -39,6 +73,7 @@ PreviousButton.propTypes = {
   variant: PropTypes.string.isRequired,
   buttonStyle: PropTypes.string.isRequired,
   isFirstUnit: PropTypes.bool.isRequired,
+  isAtTop: PropTypes.bool.isRequired,
 };
 
 export default PreviousButton;

src/index.scss

@@ -363,15 +363,13 @@
 }
 
 .top-unit-navigation {
+  display: flex;
   max-width: 100%;
   justify-content: flex-end;
 
   .next-button,
   .previous-button {
-    @media (min-width: map-get($grid-breakpoints, "md")) {
-      flex-basis: auto;
-      min-width: 8rem;
-    }
+    font-size: 1.5rem;
   }
 }