Skip to content

Commit 8c3fab3

Browse files
authored
fix: UX issues in unit page (#1913)
Fixes the following issues: * Selection behavior * Component selection is by header click only * Newly created blocks within a unit should be selected on creation/save, appear selected, and have their sidebar open * Some long text components seem to display at the default height rather than a longer height * Within the full-page unit view, the "add to collection" overflow menu item on components does not seem to work/only opens the sidebar. * Draft status indicator text is not vertically centered with icon * When reordering, dragging a short component past a long component often causes a strange stutter effect. * When dragging to reorder a component, moving quickly or scrolling often causes the drag handle to be lost / causes the block to jump somewhere else * Reordering may not consistently support a keyboard-accessible option to change order, like in course authoring * Tag button on component header opens the old tag side pane
1 parent 04e8f3a commit 8c3fab3

32 files changed

+598
-218
lines changed

codecov.yml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,4 +10,5 @@ coverage:
1010
threshold: 0%
1111
ignore:
1212
- "src/grading-settings/grading-scale/react-ranger.js"
13+
- "src/generic/DraggableList/verticalSortableList.ts"
1314
- "src/index.js"

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ const path = '/content/:contentId?/*';
2121
const mockOnClose = jest.fn();
2222
const mockSetBlockingSheet = jest.fn();
2323
const mockNavigate = jest.fn();
24+
const mockSidebarAction = jest.fn();
2425
mockContentTaxonomyTagsData.applyMock();
2526
mockTaxonomyListData.applyMock();
2627
mockTaxonomyTagsData.applyMock();
@@ -40,6 +41,11 @@ jest.mock('react-router-dom', () => ({
4041
useNavigate: () => mockNavigate,
4142
}));
4243

44+
jest.mock('../library-authoring/common/context/SidebarContext', () => ({
45+
...jest.requireActual('../library-authoring/common/context/SidebarContext'),
46+
useSidebarContext: () => ({ sidebarAction: mockSidebarAction() }),
47+
}));
48+
4349
const renderDrawer = (contentId, drawerParams = {}) => (
4450
render(
4551
<ContentTagsDrawerSheetContext.Provider value={drawerParams}>
@@ -184,6 +190,26 @@ describe('<ContentTagsDrawer />', () => {
184190
expect(screen.getByRole('button', { name: /save/i })).toBeInTheDocument();
185191
});
186192

193+
it('should change to edit mode sidebar action is set to JumpToManageTags', async () => {
194+
mockSidebarAction.mockReturnValueOnce('jump-to-manage-tags');
195+
renderDrawer(stagedTagsId, { variant: 'component' });
196+
expect(await screen.findByText('Taxonomy 1')).toBeInTheDocument();
197+
198+
// Show delete tag buttons
199+
expect(screen.getAllByRole('button', {
200+
name: /delete/i,
201+
}).length).toBe(2);
202+
203+
// Show add a tag select
204+
expect(screen.getByText(/add a tag/i)).toBeInTheDocument();
205+
206+
// Show cancel button
207+
expect(screen.getByRole('button', { name: /cancel/i })).toBeInTheDocument();
208+
209+
// Show save button
210+
expect(screen.getByRole('button', { name: /save/i })).toBeInTheDocument();
211+
});
212+
187213
it('should change to read mode when click on `Cancel` on drawer variant', async () => {
188214
renderDrawer(stagedTagsId);
189215
expect(await screen.findByText('Taxonomy 1')).toBeInTheDocument();

src/content-tags-drawer/ContentTagsDrawer.tsx

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import ContentTagsCollapsible from './ContentTagsCollapsible';
1414
import Loading from '../generic/Loading';
1515
import { useCreateContentTagsDrawerContext } from './ContentTagsDrawerHelper';
1616
import { ContentTagsDrawerContext, ContentTagsDrawerSheetContext } from './common/context';
17+
import { SidebarActions, useSidebarContext } from '../library-authoring/common/context/SidebarContext';
1718

1819
interface TaxonomyListProps {
1920
contentId: string;
@@ -244,6 +245,7 @@ const ContentTagsDrawer = ({
244245
if (contentId === undefined) {
245246
throw new Error('Error: contentId cannot be null.');
246247
}
248+
const { sidebarAction } = useSidebarContext();
247249

248250
const context = useCreateContentTagsDrawerContext(contentId, !readOnly, variant === 'drawer');
249251
const { blockingSheet } = useContext(ContentTagsDrawerSheetContext);
@@ -260,6 +262,7 @@ const ContentTagsDrawer = ({
260262
closeToast,
261263
setCollapsibleToInitalState,
262264
otherTaxonomies,
265+
toEditMode,
263266
} = context;
264267

265268
let onCloseDrawer: () => void;
@@ -302,8 +305,13 @@ const ContentTagsDrawer = ({
302305

303306
// First call of the initial collapsible states
304307
React.useEffect(() => {
305-
setCollapsibleToInitalState();
306-
}, [isTaxonomyListLoaded, isContentTaxonomyTagsLoaded]);
308+
// Open tag edit mode when sidebarAction is JumpToManageTags
309+
if (sidebarAction === SidebarActions.JumpToManageTags) {
310+
toEditMode();
311+
} else {
312+
setCollapsibleToInitalState();
313+
}
314+
}, [isTaxonomyListLoaded, isContentTaxonomyTagsLoaded, sidebarAction, toEditMode]);
307315

308316
const renderFooter = () => {
309317
if (isTaxonomyListLoaded && isContentTaxonomyTagsLoaded) {

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,15 @@ import {
77
useMutation,
88
useQueryClient,
99
} from '@tanstack/react-query';
10+
import { useParams } from 'react-router';
1011
import {
1112
getTaxonomyTagsData,
1213
getContentTaxonomyTagsData,
1314
getContentData,
1415
updateContentTaxonomyTags,
1516
getContentTaxonomyTagsCount,
1617
} from './api';
17-
import { libraryQueryPredicate, xblockQueryKeys } from '../../library-authoring/data/apiHooks';
18+
import { libraryAuthoringQueryKeys, libraryQueryPredicate, xblockQueryKeys } from '../../library-authoring/data/apiHooks';
1819
import { getLibraryId } from '../../generic/key-utils';
1920

2021
/** @typedef {import("../../taxonomy/data/types.js").TagListData} TagListData */
@@ -129,6 +130,7 @@ export const useContentData = (contentId, enabled) => (
129130
export const useContentTaxonomyTagsUpdater = (contentId) => {
130131
const queryClient = useQueryClient();
131132
const unitIframe = window.frames['xblock-iframe'];
133+
const { unitId } = useParams();
132134

133135
return useMutation({
134136
/**
@@ -158,6 +160,10 @@ export const useContentTaxonomyTagsUpdater = (contentId) => {
158160
queryClient.invalidateQueries(xblockQueryKeys.componentMetadata(contentId));
159161
// Invalidate content search to update tags count
160162
queryClient.invalidateQueries(['content_search'], { predicate: (query) => libraryQueryPredicate(query, libraryId) });
163+
// If the tags for a compoent were edited from Unit page, invalidate children query to fetch count again.
164+
if (unitId) {
165+
queryClient.invalidateQueries(libraryAuthoringQueryKeys.containerChildren(unitId));
166+
}
161167
}
162168
},
163169
onSuccess: /* istanbul ignore next */ () => {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ describe('useContentTaxonomyTagsUpdater', () => {
157157

158158
const contentId = 'testerContent';
159159
const taxonomyId = 123;
160-
const mutation = useContentTaxonomyTagsUpdater(contentId);
160+
const mutation = renderHook(() => useContentTaxonomyTagsUpdater(contentId)).result.current;
161161
const tagsData = [{
162162
taxonomy: taxonomyId,
163163
tags: ['tag1', 'tag2'],

src/course-outline/CourseOutline.test.jsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2230,7 +2230,7 @@ describe('<CourseOutline />', () => {
22302230
.reply(200, courseSectionMock);
22312231
let [subsectionElement] = await within(sectionElement).findAllByTestId('subsection-card');
22322232
const expandBtn = await within(subsectionElement).findByTestId('subsection-card-header__expanded-btn');
2233-
await userEvent.click(expandBtn);
2233+
userEvent.click(expandBtn);
22342234
const [unit] = subsection.childInfo.children;
22352235
const [unitElement] = await within(subsectionElement).findAllByTestId('unit-card');
22362236

src/generic/DraggableList/DraggableList.jsx

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
1-
import React, { useCallback } from 'react';
1+
import { useCallback } from 'react';
22
import PropTypes from 'prop-types';
33
import { createPortal } from 'react-dom';
44

55
import {
66
DndContext,
7-
closestCenter,
87
KeyboardSensor,
98
PointerSensor,
109
useSensor,
@@ -18,6 +17,7 @@ import {
1817
verticalListSortingStrategy,
1918
} from '@dnd-kit/sortable';
2019
import { restrictToVerticalAxis } from '@dnd-kit/modifiers';
20+
import { verticalSortableListCollisionDetection } from './verticalSortableList';
2121

2222
const DraggableList = ({
2323
itemList,
@@ -56,13 +56,20 @@ const DraggableList = ({
5656
setActiveId?.(event.active.id);
5757
}, [setActiveId]);
5858

59+
const handleDragCancel = useCallback(() => {
60+
setActiveId?.(null);
61+
}, [setActiveId]);
62+
5963
return (
6064
<DndContext
6165
sensors={sensors}
6266
modifiers={[restrictToVerticalAxis]}
63-
collisionDetection={closestCenter}
67+
collisionDetection={verticalSortableListCollisionDetection}
6468
onDragStart={handleDragStart}
69+
// autoScroll does not play well with verticalSortableListCollisionDetection strategy
70+
autoScroll={false}
6571
onDragEnd={handleDragEnd}
72+
onDragCancel={handleDragCancel}
6673
>
6774
<SortableContext
6875
items={itemList}

src/generic/DraggableList/SortableItem.jsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,14 +45,15 @@ const SortableItem = ({
4545
};
4646

4747
return (
48+
/* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions */
4849
<div
4950
ref={setNodeRef}
51+
onClick={onClick}
5052
>
5153
<Card
5254
style={style}
5355
className="mx-0"
5456
isClickable={isClickable}
55-
onClick={onClick}
5657
>
5758
<ActionRow style={actionStyle}>
5859
{actions}
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/* istanbul ignore file */
2+
/**
3+
This sorting strategy was copied over from https://github.com/clauderic/dnd-kit/pull/805
4+
to resolve issues with variable sized draggables.
5+
*/
6+
import { CollisionDetection, DroppableContainer } from '@dnd-kit/core';
7+
import { sortBy } from 'lodash';
8+
9+
const collision = (dropppableContainer?: DroppableContainer) => ({
10+
id: dropppableContainer?.id ?? '',
11+
value: dropppableContainer,
12+
});
13+
14+
// Look for the first (/ furthest up / highest) droppable container that is at least
15+
// 50% covered by the top edge of the dragging container.
16+
const highestDroppableContainerMajorityCovered: CollisionDetection = ({
17+
droppableContainers,
18+
collisionRect,
19+
}) => {
20+
const ascendingDroppabaleContainers = sortBy(
21+
droppableContainers,
22+
(c) => c?.rect.current?.top,
23+
);
24+
25+
for (const droppableContainer of ascendingDroppabaleContainers) {
26+
const {
27+
rect: { current: droppableRect },
28+
} = droppableContainer;
29+
30+
if (droppableRect) {
31+
const coveredPercentage = (droppableRect.top + droppableRect.height - collisionRect.top)
32+
/ droppableRect.height;
33+
34+
if (coveredPercentage > 0.5) {
35+
return [collision(droppableContainer)];
36+
}
37+
}
38+
}
39+
40+
// if we haven't found anything then we are off the top, so return the first item
41+
return [collision(ascendingDroppabaleContainers[0])];
42+
};
43+
44+
// Look for the last (/ furthest down / lowest) droppable container that is at least
45+
// 50% covered by the bottom edge of the dragging container.
46+
const lowestDroppableContainerMajorityCovered: CollisionDetection = ({
47+
droppableContainers,
48+
collisionRect,
49+
}) => {
50+
const descendingDroppabaleContainers = sortBy(
51+
droppableContainers,
52+
(c) => c?.rect.current?.top,
53+
).reverse();
54+
55+
for (const droppableContainer of descendingDroppabaleContainers) {
56+
const {
57+
rect: { current: droppableRect },
58+
} = droppableContainer;
59+
60+
if (droppableRect) {
61+
const coveredPercentage = (collisionRect.bottom - droppableRect.top) / droppableRect.height;
62+
63+
if (coveredPercentage > 0.5) {
64+
return [collision(droppableContainer)];
65+
}
66+
}
67+
}
68+
69+
// if we haven't found anything then we are off the bottom, so return the last item
70+
return [collision(descendingDroppabaleContainers[0])];
71+
};
72+
73+
export const verticalSortableListCollisionDetection: CollisionDetection = (
74+
args,
75+
) => {
76+
if (args.collisionRect.top < (args.active.rect.current?.initial?.top ?? 0)) {
77+
return highestDroppableContainerMajorityCovered(args);
78+
}
79+
return lowestDroppableContainerMajorityCovered(args);
80+
};

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,8 @@ describe('useIframeBehavior', () => {
9696
window.dispatchEvent(new MessageEvent('message', message));
9797
});
9898

99-
expect(setIframeHeight).toHaveBeenCalledWith(500);
99+
// +10 padding
100+
expect(setIframeHeight).toHaveBeenCalledWith(510);
100101
expect(setHasLoaded).toHaveBeenCalledWith(true);
101102
});
102103

0 commit comments

Comments
 (0)