Merge pull request #668 from samvera-labs/cyclical-dep-648

Remove cyclical dependence of SectionHeading and List

src/components/StructuredNavigation/NavUtils/List.js

@@ -1,7 +1,6 @@
 import React from 'react';
 import ListItem from './ListItem';
 import PropTypes from 'prop-types';
-import SectionHeading from './SectionHeading';
 
 /**
  * Build a section of structure using a <ul> HTML element, this can represent
@@ -14,36 +13,20 @@ import SectionHeading from './SectionHeading';
  * @param {Object} props.structureContainerRef React ref of the parent container
  * @param {Boolean} props.isPlaylist
  */
-const List = (({ items, sectionRef, structureContainerRef, isPlaylist }) => {
+const List = (({ items, sectionRef, structureContainerRef }) => {
   const collapsibleContent = (
     <ul
       data-testid="list"
       className="ramp--structured-nav__list"
       role="presentation"
     >
       {items.map((item, index) => {
-        // Render canvas items as SectionHeadings in non-playlist contexts
-        if (item.isCanvas && !isPlaylist) {
-          return <SectionHeading
-            key={`${item.label}-${index}`}
-            itemIndex={index + 1}
-            duration={item.duration}
-            label={item.label}
-            sectionRef={sectionRef}
-            itemId={item.id}
-            isRoot={item.isRoot}
-            structureContainerRef={structureContainerRef}
-            hasChildren={item.items?.length > 0}
-            items={item.items}
-          />;
-        } else {
-          return <ListItem
-            {...item}
-            sectionRef={sectionRef}
-            key={index}
-            structureContainerRef={structureContainerRef}
-          />;
-        }
+        return <ListItem
+          {...item}
+          sectionRef={sectionRef}
+          key={index}
+          structureContainerRef={structureContainerRef}
+        />;
       })}
     </ul>
   );
@@ -55,7 +38,6 @@ List.propTypes = {
   items: PropTypes.array.isRequired,
   sectionRef: PropTypes.object.isRequired,
   structureContainerRef: PropTypes.object.isRequired,
-  isPlaylist: PropTypes.bool.isRequired,
 };
 
 export default List;

src/components/StructuredNavigation/NavUtils/ListItem.js

@@ -1,6 +1,7 @@
 import React, { Fragment, useEffect, useRef } from 'react';
 import cx from 'classnames';
 import List from './List';
+import SectionHeading from './SectionHeading';
 import PropTypes from 'prop-types';
 import { autoScroll } from '@Services/utility-helpers';
 import { LockedSVGIcon } from '@Services/svg-icons';
@@ -36,6 +37,7 @@ const ListItem = ({
   label,
   summary,
   homepage,
+  isRoot,
   items,
   itemIndex,
   rangeId,
@@ -51,11 +53,14 @@ const ListItem = ({
     canvasDuration,
   });
 
+  // Identify item as a SectionHeading for canvases in non-playlist contexts
+  const isSectionHeading = isCanvas && !isPlaylist;
+
+  // Build rest of structure items for li items, i.e. non-SectionHeadings
   const subMenu =
-    items && items.length > 0 ? (
+    items && items.length > 0 && !isSectionHeading ? (
       <List items={items} sectionRef={sectionRef}
         structureContainerRef={structureContainerRef}
-        isPlaylist={isPlaylist}
       />
     ) : null;
 
@@ -79,34 +84,46 @@ const ListItem = ({
   const renderListItem = () => {
     return (
       <Fragment key={rangeId}>
-        <Fragment>
-          {isTitle
-            ?
-            (<span className="ramp--structured-nav__item-title"
-              role="listitem"
-              aria-label={label}
-            >
-              {label}
-            </span>)
-            : (
-              <Fragment key={id}>
-                <div className="tracker"></div>
-                {isClickable ? (
-                  <Fragment>
-                    {isEmpty && <LockedSVGIcon />}
-                    <a role="listitem"
-                      href={homepage && homepage != '' ? homepage : id}
-                      onClick={handleClick}>
-                      {`${itemIndex}. `}{label} {duration.length > 0 ? ` (${duration})` : ''}
-                    </a>
-                  </Fragment>
-                ) : (
-                  <span role="listitem" aria-label={label}>{label}</span>
-                )}
-              </Fragment>
-            )
-          }
-        </Fragment>
+        {isSectionHeading // Render items as SectionHeadings in non-playlist contexts
+          ? <SectionHeading
+            key={`${label}-${itemIndex}`}
+            itemIndex={itemIndex}
+            duration={duration}
+            label={label}
+            sectionRef={sectionRef}
+            itemId={id}
+            isRoot={isRoot}
+            structureContainerRef={structureContainerRef}
+            hasChildren={items?.length > 0}
+            items={items} />
+          : <>
+            {isTitle
+              ?
+              (<span className="ramp--structured-nav__item-title"
+                role="listitem"
+                aria-label={label}
+              >
+                {label}
+              </span>)
+              : (
+                <Fragment key={id}>
+                  <div className="tracker"></div>
+                  {isClickable ? (
+                    <>
+                      {isEmpty && <LockedSVGIcon />}
+                      <a role="listitem"
+                        href={homepage && homepage != '' ? homepage : id}
+                        onClick={handleClick}>
+                        {`${itemIndex}. `}{label} {duration.length > 0 ? ` (${duration})` : ''}
+                      </a>
+                    </>
+                  ) : (
+                    <span role="listitem" aria-label={label}>{label}</span>
+                  )}
+                </Fragment>
+              )
+            }
+          </>}
       </Fragment>
     );
   };
@@ -118,6 +135,7 @@ const ListItem = ({
         ref={liRef}
         className={cx(
           'ramp--structured-nav__list-item',
+          isSectionHeading ? 'section-list-item' : '',
           isActiveLi ? 'active' : '')
         }
         data-label={label}

src/components/StructuredNavigation/NavUtils/SectionHeading.js

@@ -30,14 +30,15 @@ const SectionHeading = ({
   sectionRef,
   structureContainerRef,
 }) => {
-  const [isOpen, setIsOpen] = useState(false);
+  // Always collapse root structure element
+  const [isOpen, setIsOpen] = useState(isRoot);
 
   const toggleOpen = (e) => {
     setIsOpen(!isOpen);
-    sectionRef.current.isOpen = true;
+    if (sectionRef.current) sectionRef.current.isOpen = true;
   };
 
-  const { isActiveSection, canvasIndex, handleClick, isPlaylist } = useActiveStructure({
+  const { isActiveSection, canvasIndex, handleClick } = useActiveStructure({
     itemIndex, isRoot,
     itemId,
     liRef: sectionRef,
@@ -104,8 +105,7 @@ const SectionHeading = ({
           items={items}
           sectionRef={sectionRef}
           key={itemId}
-          structureContainerRef={structureContainerRef}
-          isPlaylist={isPlaylist} />
+          structureContainerRef={structureContainerRef} />
       )}
     </div>
   );

src/components/StructuredNavigation/NavUtils/SectionHeading.test.js

@@ -10,6 +10,7 @@ describe('SectionHeading component', () => {
   jest.spyOn(hooks, 'useActiveStructure').mockImplementation(() => ({
     isActiveSection: false
   }));
+
   test('renders canvas with children items', () => {
     render(
       <SectionHeading
@@ -134,6 +135,7 @@ describe('SectionHeading component', () => {
         handleClick={handleClickMock}
         structureContainerRef={structureContainerRef}
         hasChildren={true}
+        items={[]}
       />
     );
     expect(screen.queryAllByTestId('listitem-section-span')).toHaveLength(1);

src/components/StructuredNavigation/StructuredNavigation.js

@@ -241,7 +241,6 @@ const StructuredNavigation = () => {
                   sectionRef={createRef()}
                   key={`${item.label}-${index}`}
                   structureContainerRef={structureContainerRef}
-                  isPlaylist={playlist.isPlaylist}
                 />
             ))
           ) : (

src/components/StructuredNavigation/StructuredNavigation.scss

@@ -18,6 +18,14 @@
     padding-top: 1em;
     color: $primaryDarker;
   }
+
+  // Remove top border for first SectionHeading (causes double lines)
+  .ramp--structured-nav__section {
+    &:first-child {
+      border-top: none;
+    }
+    border-top: 1px solid $primaryLight;
+  }
 }
 
 // Remove indentation for multiple Canvas structure with root Range
@@ -85,6 +93,13 @@ ul.ramp--structured-nav__list {
   margin: 0px;
   font-size: medium;
 
+  // CSS for nested SectionHeading in structures with root range
+  // E.g. IIIF cookbook: https://iiif.io/api/cookbook/recipe/0065-opera-multiple-canvases
+  li.section-list-item {
+    padding: 0;
+    border-top: 1px solid $primaryLight;
+  }
+
   li {
     display: block;
     padding: 0 0 0.5rem 1em;

src/components/StructuredNavigation/StructuredNavigation.test.js

@@ -53,41 +53,31 @@ describe('StructuredNavigation component', () => {
         );
       });
 
-      test('renders root Range as a span', () => {
+      test('renders root Range as a collapsible span', () => {
         expect(screen.queryByText('Table of Contents')).not.toBeNull();
 
         const rootRange = screen.getAllByTestId('listitem-section')[0];
         expect(rootRange).toHaveClass('ramp--structured-nav__section');
         expect(rootRange.children[0]).toHaveClass('section-head-buttons');
-        expect(screen.queryByTestId('listitem-section-span')).toBeInTheDocument();
-        expect(screen.queryByTestId('listitem-section-button')).not.toBeInTheDocument();
+        expect(rootRange.children[0].children[0])
+          .toHaveAttribute('data-testid', 'listitem-section-span');
+        expect(rootRange.children[0].children[0])
+          .toHaveClass('ramp--structured-nav__section-title');
 
         const sectionHead = rootRange.children[0];
         expect(sectionHead.children[0]).toHaveTextContent('Table of Contents');
         expect(sectionHead.children[0]).toHaveClass(
           'ramp--structured-nav__section-title not-clickable'
         );
-      });
-
-      test('returns a collapsible section for the root range', () => {
-        expect(screen.queryByTestId('listitem-section')).toBeInTheDocument();
-        expect(screen.queryByTestId('section-collapse-icon')).toBeInTheDocument();
-        expect(screen.queryByTestId('listitem-section-span')).toBeInTheDocument();
-        expect(screen.getByTestId('listitem-section-span')).toHaveTextContent('Table of Contents');
+        expect(rootRange.children[0].children[1]).toHaveClass('collapse-expand-button');
       });
 
       test('returns a List of items when structures are present in the manifest', () => {
-        expect(screen.queryByTestId('section-collapse-icon')).toBeInTheDocument();
-        // Collapse root range
-        fireEvent.click(screen.getByTestId('section-collapse-icon'));
         expect(screen.getAllByTestId('list').length).toBeGreaterThan(0);
       });
 
       test('first Canvas item is a section title as a button', () => {
-        expect(screen.queryByTestId('listitem-section')).toBeInTheDocument();
-        expect(screen.queryByTestId('section-collapse-icon')).toBeInTheDocument();
-        // Collapse root range
-        fireEvent.click(screen.getByTestId('section-collapse-icon'));
+        expect(screen.queryByText('Table of Contents')).toBeInTheDocument();
 
         const firstCanvas = screen.queryAllByTestId('listitem-section')[1];
         expect(firstCanvas.children[0]).toHaveTextContent('1. Lunchroom Manners11:00');
@@ -196,9 +186,6 @@ describe('StructuredNavigation component', () => {
 
       test('returns a List of items when structures are present in the manifest', () => {
         expect(screen.queryByTestId('section-collapse-icon')).toBeInTheDocument();
-        // Collapse root range
-        fireEvent.click(screen.getByTestId('section-collapse-icon'));
-
         expect(screen.getAllByTestId('list').length).toBeGreaterThan(0);
         expect(screen.queryAllByTestId('list-item')).toHaveLength(4);
       });
@@ -211,9 +198,6 @@ describe('StructuredNavigation component', () => {
           .toHaveClass('ramp--structured-nav__section-title not-clickable');
 
         expect(screen.queryByTestId('section-collapse-icon')).toBeInTheDocument();
-        // Collapse root range
-        fireEvent.click(screen.getByTestId('section-collapse-icon'));
-
         const canvasItem = screen.getAllByTestId('list-item')[0];
         expect(canvasItem.children[0]).toHaveTextContent('Atto Primo');
         expect(canvasItem.children[0]).toHaveClass(
@@ -223,15 +207,20 @@ describe('StructuredNavigation component', () => {
 
       test('renders root Range\'s descendants w/o Canvas refs as titles', () => {
         expect(screen.queryByTestId('section-collapse-icon')).toBeInTheDocument();
-        // Collapse root range
-        fireEvent.click(screen.getByTestId('section-collapse-icon'));
-
         const firstItem = screen.queryAllByTestId('list-item')[0];
         expect(firstItem.children[0].tagName).toBe('SPAN');
         expect(firstItem.children[0]).toHaveTextContent('Atto Primo');
         // First title has 2 timespans nested within
         expect(firstItem.children[1].children).toHaveLength(2);
       });
+
+      test('collapses all structure when clicked on root range collapse button', () => {
+        expect(screen.queryByTestId('section-collapse-icon')).toBeInTheDocument();
+        // Collapse root range
+        fireEvent.click(screen.getByTestId('section-collapse-icon'));
+
+        expect(screen.queryAllByTestId('list-item').length).toEqual(0);
+      });
     });
 
     describe('without structures', () => {
@@ -365,4 +354,19 @@ describe('StructuredNavigation component', () => {
       });
     });
   });
+
+  test('renders scroll to see more', () => {
+    const NavWithPlayer = withPlayerProvider(StructuredNavigation, {
+      initialState: {},
+    });
+    const NavWithManifest = withManifestProvider(NavWithPlayer, {
+      initialState: { ...manifestState(manifest) },
+    });
+    render(
+      <ErrorBoundary>
+        <NavWithManifest />
+      </ErrorBoundary>
+    );
+    expect(screen.queryByText('Scroll to see more')).toBeInTheDocument();
+  });
 });