fix: UX issues in unit page (#1913)

codecov.yml

@@ -10,4 +10,5 @@ coverage:
         threshold: 0%
 ignore:
   - "src/grading-settings/grading-scale/react-ranger.js"
+  - "src/generic/DraggableList/verticalSortableList.ts"
   - "src/index.js"

src/content-tags-drawer/ContentTagsDrawer.test.jsx

@@ -21,6 +21,7 @@ const path = '/content/:contentId?/*';
 const mockOnClose = jest.fn();
 const mockSetBlockingSheet = jest.fn();
 const mockNavigate = jest.fn();
+const mockSidebarAction = jest.fn();
 mockContentTaxonomyTagsData.applyMock();
 mockTaxonomyListData.applyMock();
 mockTaxonomyTagsData.applyMock();
@@ -40,6 +41,11 @@ jest.mock('react-router-dom', () => ({
   useNavigate: () => mockNavigate,
 }));
 
+jest.mock('../library-authoring/common/context/SidebarContext', () => ({
+  ...jest.requireActual('../library-authoring/common/context/SidebarContext'),
+  useSidebarContext: () => ({ sidebarAction: mockSidebarAction() }),
+}));
+
 const renderDrawer = (contentId, drawerParams = {}) => (
   render(
     <ContentTagsDrawerSheetContext.Provider value={drawerParams}>
@@ -184,6 +190,26 @@ describe('<ContentTagsDrawer />', () => {
     expect(screen.getByRole('button', { name: /save/i })).toBeInTheDocument();
   });
 
+  it('should change to edit mode sidebar action is set to JumpToManageTags', async () => {
+    mockSidebarAction.mockReturnValueOnce('jump-to-manage-tags');
+    renderDrawer(stagedTagsId, { variant: 'component' });
+    expect(await screen.findByText('Taxonomy 1')).toBeInTheDocument();
+
+    // Show delete tag buttons
+    expect(screen.getAllByRole('button', {
+      name: /delete/i,
+    }).length).toBe(2);
+
+    // Show add a tag select
+    expect(screen.getByText(/add a tag/i)).toBeInTheDocument();
+
+    // Show cancel button
+    expect(screen.getByRole('button', { name: /cancel/i })).toBeInTheDocument();
+
+    // Show save button
+    expect(screen.getByRole('button', { name: /save/i })).toBeInTheDocument();
+  });
+
   it('should change to read mode when click on `Cancel` on drawer variant', async () => {
     renderDrawer(stagedTagsId);
     expect(await screen.findByText('Taxonomy 1')).toBeInTheDocument();

src/content-tags-drawer/ContentTagsDrawer.tsx

@@ -14,6 +14,7 @@ import ContentTagsCollapsible from './ContentTagsCollapsible';
 import Loading from '../generic/Loading';
 import { useCreateContentTagsDrawerContext } from './ContentTagsDrawerHelper';
 import { ContentTagsDrawerContext, ContentTagsDrawerSheetContext } from './common/context';
+import { SidebarActions, useSidebarContext } from '../library-authoring/common/context/SidebarContext';
 
 interface TaxonomyListProps {
   contentId: string;
@@ -244,6 +245,7 @@ const ContentTagsDrawer = ({
   if (contentId === undefined) {
     throw new Error('Error: contentId cannot be null.');
   }
+  const { sidebarAction } = useSidebarContext();
 
   const context = useCreateContentTagsDrawerContext(contentId, !readOnly, variant === 'drawer');
   const { blockingSheet } = useContext(ContentTagsDrawerSheetContext);
@@ -260,6 +262,7 @@ const ContentTagsDrawer = ({
     closeToast,
     setCollapsibleToInitalState,
     otherTaxonomies,
+    toEditMode,
   } = context;
 
   let onCloseDrawer: () => void;
@@ -302,8 +305,13 @@ const ContentTagsDrawer = ({
 
   // First call of the initial collapsible states
   React.useEffect(() => {
-    setCollapsibleToInitalState();
-  }, [isTaxonomyListLoaded, isContentTaxonomyTagsLoaded]);
+    // Open tag edit mode when sidebarAction is JumpToManageTags
+    if (sidebarAction === SidebarActions.JumpToManageTags) {
+      toEditMode();
+    } else {
+      setCollapsibleToInitalState();
+    }
+  }, [isTaxonomyListLoaded, isContentTaxonomyTagsLoaded, sidebarAction, toEditMode]);
 
   const renderFooter = () => {
     if (isTaxonomyListLoaded && isContentTaxonomyTagsLoaded) {

src/content-tags-drawer/data/apiHooks.jsx

@@ -7,14 +7,15 @@ import {
   useMutation,
   useQueryClient,
 } from '@tanstack/react-query';
+import { useParams } from 'react-router';
 import {
   getTaxonomyTagsData,
   getContentTaxonomyTagsData,
   getContentData,
   updateContentTaxonomyTags,
   getContentTaxonomyTagsCount,
 } from './api';
-import { libraryQueryPredicate, xblockQueryKeys } from '../../library-authoring/data/apiHooks';
+import { libraryAuthoringQueryKeys, libraryQueryPredicate, xblockQueryKeys } from '../../library-authoring/data/apiHooks';
 import { getLibraryId } from '../../generic/key-utils';
 
 /** @typedef {import("../../taxonomy/data/types.js").TagListData} TagListData */
@@ -129,6 +130,7 @@ export const useContentData = (contentId, enabled) => (
 export const useContentTaxonomyTagsUpdater = (contentId) => {
   const queryClient = useQueryClient();
   const unitIframe = window.frames['xblock-iframe'];
+  const { unitId } = useParams();
 
   return useMutation({
     /**
@@ -158,6 +160,10 @@ export const useContentTaxonomyTagsUpdater = (contentId) => {
         queryClient.invalidateQueries(xblockQueryKeys.componentMetadata(contentId));
         // Invalidate content search to update tags count
         queryClient.invalidateQueries(['content_search'], { predicate: (query) => libraryQueryPredicate(query, libraryId) });
+        // If the tags for a compoent were edited from Unit page, invalidate children query to fetch count again.
+        if (unitId) {
+          queryClient.invalidateQueries(libraryAuthoringQueryKeys.containerChildren(unitId));
+        }
       }
     },
     onSuccess: /* istanbul ignore next */ () => {

src/content-tags-drawer/data/apiHooks.test.jsx

@@ -157,7 +157,7 @@ describe('useContentTaxonomyTagsUpdater', () => {
 
     const contentId = 'testerContent';
     const taxonomyId = 123;
-    const mutation = useContentTaxonomyTagsUpdater(contentId);
+    const mutation = renderHook(() => useContentTaxonomyTagsUpdater(contentId)).result.current;
     const tagsData = [{
       taxonomy: taxonomyId,
       tags: ['tag1', 'tag2'],

src/course-outline/CourseOutline.test.jsx

@@ -2230,7 +2230,7 @@ describe('<CourseOutline />', () => {
       .reply(200, courseSectionMock);
     let [subsectionElement] = await within(sectionElement).findAllByTestId('subsection-card');
     const expandBtn = await within(subsectionElement).findByTestId('subsection-card-header__expanded-btn');
-    await userEvent.click(expandBtn);
+    userEvent.click(expandBtn);
     const [unit] = subsection.childInfo.children;
     const [unitElement] = await within(subsectionElement).findAllByTestId('unit-card');
 

src/generic/DraggableList/DraggableList.jsx

@@ -1,10 +1,9 @@
-import React, { useCallback } from 'react';
+import { useCallback } from 'react';
 import PropTypes from 'prop-types';
 import { createPortal } from 'react-dom';
 
 import {
   DndContext,
-  closestCenter,
   KeyboardSensor,
   PointerSensor,
   useSensor,
@@ -18,6 +17,7 @@ import {
   verticalListSortingStrategy,
 } from '@dnd-kit/sortable';
 import { restrictToVerticalAxis } from '@dnd-kit/modifiers';
+import { verticalSortableListCollisionDetection } from './verticalSortableList';
 
 const DraggableList = ({
   itemList,
@@ -56,13 +56,20 @@ const DraggableList = ({
     setActiveId?.(event.active.id);
   }, [setActiveId]);
 
+  const handleDragCancel = useCallback(() => {
+    setActiveId?.(null);
+  }, [setActiveId]);
+
   return (
     <DndContext
       sensors={sensors}
       modifiers={[restrictToVerticalAxis]}
-      collisionDetection={closestCenter}
+      collisionDetection={verticalSortableListCollisionDetection}
       onDragStart={handleDragStart}
+      // autoScroll does not play well with verticalSortableListCollisionDetection strategy
+      autoScroll={false}
       onDragEnd={handleDragEnd}
+      onDragCancel={handleDragCancel}
     >
       <SortableContext
         items={itemList}

src/generic/DraggableList/SortableItem.jsx

@@ -45,14 +45,15 @@ const SortableItem = ({
   };
 
   return (
+    /* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */
     <div
       ref={setNodeRef}
+      onClick={onClick}
     >
       <Card
         style={style}
         className="mx-0"
         isClickable={isClickable}
-        onClick={onClick}
       >
         <ActionRow style={actionStyle}>
           {actions}

src/generic/DraggableList/verticalSortableList.ts

@@ -0,0 +1,80 @@
+/* istanbul ignore file */
+/**
+This sorting strategy was copied over from https://github.com/clauderic/dnd-kit/pull/805
+to resolve issues with variable sized draggables.
+*/
+import { CollisionDetection, DroppableContainer } from '@dnd-kit/core';
+import { sortBy } from 'lodash';
+
+const collision = (dropppableContainer?: DroppableContainer) => ({
+  id: dropppableContainer?.id ?? '',
+  value: dropppableContainer,
+});
+
+// Look for the first (/ furthest up / highest) droppable container that is at least
+// 50% covered by the top edge of the dragging container.
+const highestDroppableContainerMajorityCovered: CollisionDetection = ({
+  droppableContainers,
+  collisionRect,
+}) => {
+  const ascendingDroppabaleContainers = sortBy(
+    droppableContainers,
+    (c) => c?.rect.current?.top,
+  );
+
+  for (const droppableContainer of ascendingDroppabaleContainers) {
+    const {
+      rect: { current: droppableRect },
+    } = droppableContainer;
+
+    if (droppableRect) {
+      const coveredPercentage = (droppableRect.top + droppableRect.height - collisionRect.top)
+        / droppableRect.height;
+
+      if (coveredPercentage > 0.5) {
+        return [collision(droppableContainer)];
+      }
+    }
+  }
+
+  // if we haven't found anything then we are off the top, so return the first item
+  return [collision(ascendingDroppabaleContainers[0])];
+};
+
+// Look for the last (/ furthest down / lowest) droppable container that is at least
+// 50% covered by the bottom edge of the dragging container.
+const lowestDroppableContainerMajorityCovered: CollisionDetection = ({
+  droppableContainers,
+  collisionRect,
+}) => {
+  const descendingDroppabaleContainers = sortBy(
+    droppableContainers,
+    (c) => c?.rect.current?.top,
+  ).reverse();
+
+  for (const droppableContainer of descendingDroppabaleContainers) {
+    const {
+      rect: { current: droppableRect },
+    } = droppableContainer;
+
+    if (droppableRect) {
+      const coveredPercentage = (collisionRect.bottom - droppableRect.top) / droppableRect.height;
+
+      if (coveredPercentage > 0.5) {
+        return [collision(droppableContainer)];
+      }
+    }
+  }
+
+  // if we haven't found anything then we are off the bottom, so return the last item
+  return [collision(descendingDroppabaleContainers[0])];
+};
+
+export const verticalSortableListCollisionDetection: CollisionDetection = (
+  args,
+) => {
+  if (args.collisionRect.top < (args.active.rect.current?.initial?.top ?? 0)) {
+    return highestDroppableContainerMajorityCovered(args);
+  }
+  return lowestDroppableContainerMajorityCovered(args);
+};

src/generic/hooks/tests/hooks.test.tsx

@@ -96,7 +96,8 @@ describe('useIframeBehavior', () => {
       window.dispatchEvent(new MessageEvent('message', message));
     });
 
-    expect(setIframeHeight).toHaveBeenCalledWith(500);
+    // +10 padding
+    expect(setIframeHeight).toHaveBeenCalledWith(510);
     expect(setHasLoaded).toHaveBeenCalledWith(true);
   });
 

src/generic/hooks/useIframeBehavior.tsx

@@ -46,7 +46,8 @@ export const useIframeBehavior = ({
 
     switch (type) {
       case iframeMessageTypes.resize:
-        setIframeHeight(payload.height);
+        // Adding 10px as padding
+        setIframeHeight(payload.height + 10);
         if (!hasLoaded && iframeHeight === 0 && payload.height > 0) {
           setHasLoaded(true);
         }

src/library-authoring/LibraryAuthoringPage.test.tsx

@@ -55,6 +55,10 @@ const path = '/library/:libraryId/*';
 const libraryTitle = mockContentLibrary.libraryData.title;
 
 describe('<LibraryAuthoringPage />', () => {
+  beforeAll(() => {
+    jest.useFakeTimers();
+  });
+
   beforeEach(async () => {
     const mocks = initializeMocks();
     axiosMock = mocks.axiosMock;
@@ -78,6 +82,10 @@ describe('<LibraryAuthoringPage />', () => {
     });
   });
 
+  afterAll(() => {
+    jest.useRealTimers();
+  });
+
   const renderLibraryPage = async () => {
     render(<LibraryLayout />, { path, params: { libraryId: mockContentLibrary.libraryId } });
 
@@ -392,7 +400,7 @@ describe('<LibraryAuthoringPage />', () => {
     await waitFor(() => expect(screen.queryByTestId('library-sidebar')).not.toBeInTheDocument());
   });
 
-  it('should open component sidebar, showing manage tab on clicking add to collection menu item (component)', async () => {
+  it('should open component sidebar, showing manage tab on clicking add to collection menu item - component', async () => {
     const mockResult0 = { ...mockResult }.results[0].hits[0];
     const displayName = 'Introduction to Testing';
     expect(mockResult0.display_name).toStrictEqual(displayName);
@@ -407,17 +415,18 @@ describe('<LibraryAuthoringPage />', () => {
 
     const sidebar = screen.getByTestId('library-sidebar');
 
-    const { getByRole, queryByText } = within(sidebar);
+    const { getByRole, findByText } = within(sidebar);
 
-    await waitFor(() => expect(queryByText(displayName)).toBeInTheDocument());
+    expect(await findByText(displayName)).toBeInTheDocument();
+    jest.advanceTimersByTime(300);
     expect(getByRole('tab', { selected: true })).toHaveTextContent('Manage');
     const closeButton = getByRole('button', { name: /close/i });
     fireEvent.click(closeButton);
 
     await waitFor(() => expect(screen.queryByTestId('library-sidebar')).not.toBeInTheDocument());
   });
 
-  it('should open component sidebar, showing manage tab on clicking add to collection menu item (unit)', async () => {
+  it('should open component sidebar, showing manage tab on clicking add to collection menu item - unit', async () => {
     const displayName = 'Test Unit';
     await renderLibraryPage();
 
@@ -430,9 +439,10 @@ describe('<LibraryAuthoringPage />', () => {
 
     const sidebar = screen.getByTestId('library-sidebar');
 
-    const { getByRole, queryByText } = within(sidebar);
+    const { getByRole, findByText } = within(sidebar);
 
-    await waitFor(() => expect(queryByText(displayName)).toBeInTheDocument());
+    expect(await findByText(displayName)).toBeInTheDocument();
+    jest.advanceTimersByTime(300);
     expect(getByRole('tab', { selected: true })).toHaveTextContent('Manage');
     const closeButton = getByRole('button', { name: /close/i });
     fireEvent.click(closeButton);