diff --git a/entry_types/scrolled/config/locales/de.yml b/entry_types/scrolled/config/locales/de.yml index 4210f71956..93052ddad2 100644 --- a/entry_types/scrolled/config/locales/de.yml +++ b/entry_types/scrolled/config/locales/de.yml @@ -1926,6 +1926,17 @@ de: review: add_comment: Kommentar hinzufügen cancel_add_comment: Abbrechen + comment_toolbar: Kommentare + filter: + label: Kommentare filtern + unresolved: Ungelöst + all: Alle + previous_comment: Vorheriger Kommentar + next_comment: Nächster Kommentar + comment_count: + zero: Keine Kommentare + one: 1 Kommentar + other: '%{count} Kommentare' select_content_element: Zum Kommentieren auswählen select_section: Abschnitt zum Kommentieren auswählen select_text_to_comment: Text zum Kommentieren auswählen diff --git a/entry_types/scrolled/config/locales/en.yml b/entry_types/scrolled/config/locales/en.yml index 3e7b8f3a24..acf8f3da58 100644 --- a/entry_types/scrolled/config/locales/en.yml +++ b/entry_types/scrolled/config/locales/en.yml @@ -1755,6 +1755,17 @@ en: review: add_comment: Add comment cancel_add_comment: Cancel + comment_toolbar: Comments + filter: + label: Filter comments + unresolved: Unresolved + all: All + previous_comment: Previous comment + next_comment: Next comment + comment_count: + zero: No comments + one: 1 comment + other: '%{count} comments' select_content_element: Select to comment select_section: Select section to comment select_text_to_comment: Select text to comment diff --git a/entry_types/scrolled/package/spec/contentElements/textBlock/editor-spec.js b/entry_types/scrolled/package/spec/contentElements/textBlock/editor-spec.js index e746be46aa..7956218d44 100644 --- a/entry_types/scrolled/package/spec/contentElements/textBlock/editor-spec.js +++ b/entry_types/scrolled/package/spec/contentElements/textBlock/editor-spec.js @@ -129,48 +129,4 @@ describe('contentElements/textBlock/editor', () => { }); }); - describe('#compareRanges', () => { - function range(anchor, focus) { - return { - anchor: {path: [anchor[0], anchor[1]], offset: anchor[2]}, - focus: {path: [focus[0], focus[1]], offset: focus[2]} - }; - } - - it('orders ranges by start block', () => { - const earlier = range([0, 0, 0], [0, 0, 5]); - const later = range([1, 0, 0], [1, 0, 5]); - - expect(type.compareRanges(earlier, later)).toBeLessThan(0); - expect(type.compareRanges(later, earlier)).toBeGreaterThan(0); - }); - - it('orders ranges within the same block by offset', () => { - const earlier = range([0, 0, 2], [0, 0, 4]); - const later = range([0, 0, 6], [0, 0, 8]); - - expect(type.compareRanges(earlier, later)).toBeLessThan(0); - }); - - it('uses the smaller of anchor and focus as the start point', () => { - const reversed = range([0, 0, 8], [0, 0, 2]); - const forward = range([0, 0, 5], [0, 0, 6]); - - expect(type.compareRanges(reversed, forward)).toBeLessThan(0); - }); - - it('returns 0 for equal ranges', () => { - const r = range([0, 0, 0], [0, 0, 5]); - - expect(type.compareRanges(r, r)).toBe(0); - }); - - it('sorts range-less subjects last', () => { - const r = range([0, 0, 0], [0, 0, 5]); - - expect(type.compareRanges(undefined, r)).toBeGreaterThan(0); - expect(type.compareRanges(r, undefined)).toBeLessThan(0); - expect(type.compareRanges(undefined, undefined)).toBe(0); - }); - }); }); diff --git a/entry_types/scrolled/package/spec/contentElements/textBlock/review-spec.js b/entry_types/scrolled/package/spec/contentElements/textBlock/review-spec.js new file mode 100644 index 0000000000..6ac080a81f --- /dev/null +++ b/entry_types/scrolled/package/spec/contentElements/textBlock/review-spec.js @@ -0,0 +1,49 @@ +import 'contentElements/textBlock/review'; +import {review} from 'review'; + +describe('contentElements/textBlock/review', () => { + const compareRanges = review.contentElementTypes.findCompareRanges('textBlock'); + + function range(anchor, focus) { + return { + anchor: {path: [anchor[0], anchor[1]], offset: anchor[2]}, + focus: {path: [focus[0], focus[1]], offset: focus[2]} + }; + } + + it('orders ranges by start block', () => { + const earlier = range([0, 0, 0], [0, 0, 5]); + const later = range([1, 0, 0], [1, 0, 5]); + + expect(compareRanges(earlier, later)).toBeLessThan(0); + expect(compareRanges(later, earlier)).toBeGreaterThan(0); + }); + + it('orders ranges within the same block by offset', () => { + const earlier = range([0, 0, 2], [0, 0, 4]); + const later = range([0, 0, 6], [0, 0, 8]); + + expect(compareRanges(earlier, later)).toBeLessThan(0); + }); + + it('uses the smaller of anchor and focus as the start point', () => { + const reversed = range([0, 0, 8], [0, 0, 2]); + const forward = range([0, 0, 5], [0, 0, 6]); + + expect(compareRanges(reversed, forward)).toBeLessThan(0); + }); + + it('returns 0 for equal ranges', () => { + const r = range([0, 0, 0], [0, 0, 5]); + + expect(compareRanges(r, r)).toBe(0); + }); + + it('sorts range-less subjects last', () => { + const r = range([0, 0, 0], [0, 0, 5]); + + expect(compareRanges(undefined, r)).toBeGreaterThan(0); + expect(compareRanges(r, undefined)).toBeLessThan(0); + expect(compareRanges(undefined, undefined)).toBe(0); + }); +}); diff --git a/entry_types/scrolled/package/spec/editor/views/EntryCommentsView-spec.js b/entry_types/scrolled/package/spec/editor/views/EntryCommentsView-spec.js index e80551e145..8ba388a135 100644 --- a/entry_types/scrolled/package/spec/editor/views/EntryCommentsView-spec.js +++ b/entry_types/scrolled/package/spec/editor/views/EntryCommentsView-spec.js @@ -3,6 +3,7 @@ import userEvent from '@testing-library/user-event'; import {act} from '@testing-library/react'; import {editor} from 'pageflow-scrolled/editor'; +import {review} from 'pageflow-scrolled/review'; import {EntryCommentsView} from 'editor/views/EntryCommentsView'; import styles from 'editor/views/EntryCommentsView.module.css'; @@ -14,7 +15,7 @@ describe('EntryCommentsView', () => { const {createEntry} = useEditorGlobals(); beforeAll(() => { - editor.contentElementTypes.register('fixture', { + review.contentElementTypes.register('fixture', { compareRanges: (a, b) => (a?.start ?? 0) - (b?.start ?? 0) }); }); diff --git a/entry_types/scrolled/package/spec/editor/views/SelectionCommentsView-spec.js b/entry_types/scrolled/package/spec/editor/views/SelectionCommentsView-spec.js index 7c0a873dc1..335af9431a 100644 --- a/entry_types/scrolled/package/spec/editor/views/SelectionCommentsView-spec.js +++ b/entry_types/scrolled/package/spec/editor/views/SelectionCommentsView-spec.js @@ -2,6 +2,7 @@ import '@testing-library/jest-dom/extend-expect'; import userEvent from '@testing-library/user-event'; import {editor} from 'pageflow-scrolled/editor'; +import {review} from 'pageflow-scrolled/review'; import {SelectionCommentsView} from 'editor/views/SelectionCommentsView'; import {factories, useFakeTranslations, renderBackboneView} from 'pageflow/testHelpers'; @@ -12,7 +13,7 @@ describe('SelectionCommentsView', () => { const {createEntry} = useEditorGlobals(); beforeAll(() => { - editor.contentElementTypes.register('fixture', { + review.contentElementTypes.register('fixture', { compareRanges: (a, b) => (a?.start ?? 0) - (b?.start ?? 0) }); }); diff --git a/entry_types/scrolled/package/spec/frontend/commenting/features/commentDisplayFilter-spec.js b/entry_types/scrolled/package/spec/frontend/commenting/features/commentDisplayFilter-spec.js new file mode 100644 index 0000000000..b91f4fb491 --- /dev/null +++ b/entry_types/scrolled/package/spec/frontend/commenting/features/commentDisplayFilter-spec.js @@ -0,0 +1,131 @@ +import '@testing-library/jest-dom/extend-expect'; +import {fireEvent, within} from '@testing-library/react'; + +import {renderEntry, useCommentingPageObjects} from 'support/pageObjects/commenting'; + +describe('comment display filter', () => { + useCommentingPageObjects(); + + beforeEach(() => { + window.HTMLElement.prototype.scrollIntoView = jest.fn(); + }); + + it('expands resolved comments in the popover when showing all', () => { + const entry = renderEntry({ + seed: {contentElements: [{typeName: 'withTestId', configuration: {testId: 5}}]}, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 1, resolvedAt: '2026-01-01', + comments: [{id: 10, body: 'Resolved comment', creatorName: 'Bob', creatorId: 2}]} + ] + } + }); + + fireEvent.click(entry.getCommentFilterButton('all')); + fireEvent.click(entry.getAllCommentBadges()[0]); + + expect(entry.getByText('Resolved comment')).toBeInTheDocument(); + expect(entry.queryByPlaceholderText('Add a comment...')).not.toBeInTheDocument(); + }); + + it('shows the unresolved and total comment counts in the segments', () => { + const entry = renderEntry({ + seed: { + contentElements: [ + {typeName: 'withTestId', configuration: {testId: 5}}, + {typeName: 'withTestId', configuration: {testId: 6}}, + {typeName: 'withTestId', configuration: {testId: 7}} + ] + }, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 1, resolvedAt: null, + comments: [{id: 10, body: 'a', creatorName: 'Bob', creatorId: 2}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 2, resolvedAt: null, + comments: [{id: 11, body: 'b', creatorName: 'Bob', creatorId: 2}]}, + {id: 3, subjectType: 'ContentElement', subjectId: 3, resolvedAt: '2026-01-01', + comments: [{id: 12, body: 'c', creatorName: 'Bob', creatorId: 2}]} + ] + } + }); + + expect(within(entry.getCommentFilterButton('unresolved')).getByText('2')) + .toBeInTheDocument(); + expect(within(entry.getCommentFilterButton('all')).getByText('3')) + .toBeInTheDocument(); + }); + + it('shows unresolved comments with the unresolved segment active by default', () => { + const entry = renderEntryWithResolvedThread(); + + expect(entry.queryAllCommentBadges()).toHaveLength(0); + expect(entry.getCommentFilterButton('unresolved')) + .toHaveAttribute('aria-pressed', 'true'); + expect(entry.getCommentFilterButton('all')) + .toHaveAttribute('aria-pressed', 'false'); + }); + + it('shows resolved comments when selecting all', () => { + const entry = renderEntryWithResolvedThread(); + + fireEvent.click(entry.getCommentFilterButton('all')); + + expect(entry.queryAllCommentBadges()).toHaveLength(1); + expect(entry.getCommentFilterButton('all')) + .toHaveAttribute('aria-pressed', 'true'); + }); + + it('shows both resolved and unresolved comments when showing all', () => { + const entry = renderEntry({ + seed: { + contentElements: [ + {typeName: 'withTestId', configuration: {testId: 5}}, + {typeName: 'withTestId', configuration: {testId: 6}} + ] + }, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 1, resolvedAt: null, + comments: [{id: 10, body: 'Open', creatorName: 'Bob', creatorId: 2}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 2, resolvedAt: '2026-01-01', + comments: [{id: 11, body: 'Done', creatorName: 'Bob', creatorId: 2}]} + ] + } + }); + + expect(entry.queryAllCommentBadges()).toHaveLength(1); + + fireEvent.click(entry.getCommentFilterButton('all')); + + expect(entry.queryAllCommentBadges()).toHaveLength(2); + }); + + it('hides resolved comments again when selecting unresolved', () => { + const entry = renderEntryWithResolvedThread(); + + fireEvent.click(entry.getCommentFilterButton('all')); + expect(entry.queryAllCommentBadges()).toHaveLength(1); + + fireEvent.click(entry.getCommentFilterButton('unresolved')); + + expect(entry.queryAllCommentBadges()).toHaveLength(0); + }); + + function renderEntryWithResolvedThread() { + return renderEntry({ + seed: { + contentElements: [{typeName: 'withTestId', configuration: {testId: 5}}] + }, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 1, resolvedAt: '2026-01-01', + comments: [{id: 10, body: 'Done', creatorName: 'Bob', creatorId: 2}]} + ] + } + }); + } +}); diff --git a/entry_types/scrolled/package/spec/frontend/commenting/features/commentNavigation-spec.js b/entry_types/scrolled/package/spec/frontend/commenting/features/commentNavigation-spec.js new file mode 100644 index 0000000000..5fb69dbed2 --- /dev/null +++ b/entry_types/scrolled/package/spec/frontend/commenting/features/commentNavigation-spec.js @@ -0,0 +1,156 @@ +import '@testing-library/jest-dom/extend-expect'; +import {fireEvent, within} from '@testing-library/react'; + +import {renderEntry, useCommentingPageObjects} from 'support/pageObjects/commenting'; + +describe('comment navigation', () => { + useCommentingPageObjects(); + + beforeEach(() => { + window.HTMLElement.prototype.scrollIntoView = jest.fn(); + }); + + it('shows a dash in the position indicator before stepping', () => { + const entry = renderEntryWithTwoThreads(); + + expect(within(entry.getCommentToolbar()).getByText('– /')).toBeInTheDocument(); + }); + + it('opens each thread popover when cycling forward', () => { + const entry = renderEntryWithTwoThreads(); + + const next = entry.getNextCommentButton(); + + fireEvent.click(next); + expect(entry.getByText('First comment')).toBeInTheDocument(); + + fireEvent.click(next); + expect(entry.getByText('Second comment')).toBeInTheDocument(); + expect(entry.queryByText('First comment')).not.toBeInTheDocument(); + }); + + it('wraps to the last thread when cycling backward from the start', () => { + const entry = renderEntryWithTwoThreads(); + + fireEvent.click(entry.getPreviousCommentButton()); + + expect(entry.getByText('Second comment')).toBeInTheDocument(); + }); + + it('disables navigation when there are no comments for the filter', () => { + const entry = renderEntry({ + seed: {contentElements: [{typeName: 'withTestId', configuration: {testId: 5}}]}, + commenting: {currentUser: {id: 42, name: 'Alice'}, commentThreads: []} + }); + + expect(entry.getNextCommentButton()).toBeDisabled(); + expect(entry.getPreviousCommentButton()).toBeDisabled(); + }); + + it('skips resolved comments while navigating', () => { + const entry = renderEntry({ + seed: { + contentElements: [ + {typeName: 'withTestId', configuration: {testId: 5}}, + {typeName: 'withTestId', configuration: {testId: 6}}, + {typeName: 'withTestId', configuration: {testId: 7}} + ] + }, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 1, resolvedAt: null, + comments: [{id: 10, body: 'First comment', creatorName: 'Bob', creatorId: 2}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 2, resolvedAt: '2026-01-01', + comments: [{id: 11, body: 'Resolved comment', creatorName: 'Bob', creatorId: 2}]}, + {id: 3, subjectType: 'ContentElement', subjectId: 3, resolvedAt: null, + comments: [{id: 12, body: 'Third comment', creatorName: 'Bob', creatorId: 2}]} + ] + } + }); + + const next = entry.getNextCommentButton(); + + fireEvent.click(next); + expect(entry.getByText('First comment')).toBeInTheDocument(); + + fireEvent.click(next); + expect(entry.getByText('Third comment')).toBeInTheDocument(); + expect(entry.queryByText('Resolved comment')).not.toBeInTheDocument(); + }); + + it('shows the current position while stepping', () => { + const entry = renderEntryWithTwoThreads(); + + const toolbar = entry.getCommentToolbar(); + const next = entry.getNextCommentButton(); + + fireEvent.click(next); + expect(within(toolbar).getByText('1 /')).toBeInTheDocument(); + + fireEvent.click(next); + expect(within(toolbar).getByText('2 /')).toBeInTheDocument(); + }); + + it('highlights the current thread when stepping within one element', () => { + const entry = renderEntryWithTwoThreadsOnOneElement(); + + const next = entry.getNextCommentButton(); + + fireEvent.click(next); + expect(entry.getByText('First thread').closest('[aria-current="true"]')).not.toBeNull(); + expect(entry.getByText('Second thread').closest('[aria-current="true"]')).toBeNull(); + + fireEvent.click(next); + expect(entry.getByText('Second thread').closest('[aria-current="true"]')).not.toBeNull(); + expect(entry.getByText('First thread').closest('[aria-current="true"]')).toBeNull(); + }); + + it('keeps the popover open when pressing a toolbar button', () => { + const entry = renderEntryWithTwoThreadsOnOneElement(); + + const next = entry.getNextCommentButton(); + + fireEvent.click(next); + expect(entry.getByText('First thread')).toBeInTheDocument(); + + fireEvent.mouseDown(next); + + expect(entry.getByText('First thread')).toBeInTheDocument(); + }); + + function renderEntryWithTwoThreadsOnOneElement() { + return renderEntry({ + seed: {contentElements: [{typeName: 'withTestId', configuration: {testId: 5}}]}, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 1, resolvedAt: null, + comments: [{id: 10, body: 'First thread', creatorName: 'Bob', creatorId: 2}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 1, resolvedAt: null, + comments: [{id: 11, body: 'Second thread', creatorName: 'Bob', creatorId: 2}]} + ] + } + }); + } + + function renderEntryWithTwoThreads() { + return renderEntry({ + seed: { + contentElements: [ + {typeName: 'withTestId', configuration: {testId: 5}}, + {typeName: 'withTestId', configuration: {testId: 6}} + ] + }, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 1, resolvedAt: null, + comments: [{id: 10, body: 'First comment', creatorName: 'Bob', creatorId: 2}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 2, resolvedAt: null, + comments: [{id: 11, body: 'Second comment', creatorName: 'Bob', creatorId: 2}]} + ] + } + }); + } +}); diff --git a/entry_types/scrolled/package/spec/frontend/commenting/features/commentNavigationExcursions-spec.js b/entry_types/scrolled/package/spec/frontend/commenting/features/commentNavigationExcursions-spec.js new file mode 100644 index 0000000000..f19d94be5e --- /dev/null +++ b/entry_types/scrolled/package/spec/frontend/commenting/features/commentNavigationExcursions-spec.js @@ -0,0 +1,83 @@ +import React from 'react'; +import '@testing-library/jest-dom/extend-expect'; +import {fireEvent} from '@testing-library/react'; + +import {frontend} from 'frontend'; +import {renderEntry, useCommentingPageObjects} from 'support/pageObjects/commenting'; + +describe('comment navigation across excursions', () => { + useCommentingPageObjects(); + + beforeEach(() => { + window.HTMLElement.prototype.scrollIntoView = jest.fn(); + + frontend.widgetTypes.register('excursionOverlay', { + component: ({children}) =>
{children}
+ }); + }); + + const seed = { + widgets: [{typeName: 'excursionOverlay', role: 'excursion'}], + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 1, storylineId: 1, configuration: {}}, + {id: 2, storylineId: 2, configuration: {title: 'excursion'}} + ], + sections: [ + {id: 1, permaId: 100, chapterId: 1}, + {id: 2, permaId: 200, chapterId: 2} + ], + contentElements: [ + {id: 1, permaId: 1, sectionId: 1, typeName: 'withTestId', configuration: {testId: 5}}, + {id: 2, permaId: 2, sectionId: 2, typeName: 'withTestId', configuration: {testId: 6}} + ] + }; + + it('activates the excursion and opens the popover when navigating to its comment', () => { + const entry = renderEntry({ + seed, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 2, resolvedAt: null, + comments: [{id: 10, body: 'Excursion comment', creatorName: 'Bob', creatorId: 2}]} + ] + } + }); + + expect(entry.queryByText('Excursion comment')).not.toBeInTheDocument(); + + fireEvent.click(entry.getNextCommentButton()); + + expect(entry.getByText('Excursion comment')).toBeInTheDocument(); + }); + + it('returns from the excursion when navigating onward to a main storyline comment', () => { + const entry = renderEntry({ + seed, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 1, resolvedAt: null, + comments: [{id: 10, body: 'Main comment', creatorName: 'Bob', creatorId: 2}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 2, resolvedAt: null, + comments: [{id: 11, body: 'Excursion comment', creatorName: 'Bob', creatorId: 2}]} + ] + } + }); + + const next = entry.getNextCommentButton(); + + fireEvent.click(next); + fireEvent.click(next); + expect(entry.getByText('Excursion comment')).toBeInTheDocument(); + + fireEvent.click(next); + + expect(entry.queryByText('Excursion comment')).not.toBeInTheDocument(); + expect(entry.getByText('Main comment')).toBeInTheDocument(); + }); +}); diff --git a/entry_types/scrolled/package/spec/frontend/commenting/features/commentNavigationSelection-spec.js b/entry_types/scrolled/package/spec/frontend/commenting/features/commentNavigationSelection-spec.js new file mode 100644 index 0000000000..9c443b7ba4 --- /dev/null +++ b/entry_types/scrolled/package/spec/frontend/commenting/features/commentNavigationSelection-spec.js @@ -0,0 +1,70 @@ +import '@testing-library/jest-dom/extend-expect'; +import {fireEvent} from '@testing-library/react'; + +import {renderEntry, useCommentingPageObjects} from 'support/pageObjects/commenting'; +import styles from 'frontend/commenting/FloatingToolbar.module.css'; + +describe('comment navigation coupled with selection', () => { + useCommentingPageObjects(); + + beforeEach(() => { + window.HTMLElement.prototype.scrollIntoView = jest.fn(); + }); + + it('moves the position indicator to the clicked subject', () => { + const entry = renderEntryWithThreeThreads(); + + fireEvent.click(entry.getAllCommentBadges()[1]); + + const toolbar = entry.getCommentToolbar(); + + expect(toolbar.querySelector(`.${styles.count}`)).toHaveTextContent(/^2\s*\//); + }); + + it('continues navigation forward from the clicked subject', () => { + const entry = renderEntryWithThreeThreads(); + + fireEvent.click(entry.getAllCommentBadges()[1]); + expect(entry.getByText('Second comment')).toBeInTheDocument(); + + fireEvent.click(entry.getNextCommentButton()); + + expect(entry.getByText('Third comment')).toBeInTheDocument(); + expect(entry.queryByText('Second comment')).not.toBeInTheDocument(); + }); + + it('continues navigation backward from the clicked subject', () => { + const entry = renderEntryWithThreeThreads(); + + fireEvent.click(entry.getAllCommentBadges()[1]); + expect(entry.getByText('Second comment')).toBeInTheDocument(); + + fireEvent.click(entry.getPreviousCommentButton()); + + expect(entry.getByText('First comment')).toBeInTheDocument(); + expect(entry.queryByText('Second comment')).not.toBeInTheDocument(); + }); + + function renderEntryWithThreeThreads() { + return renderEntry({ + seed: { + contentElements: [ + {typeName: 'withTestId', configuration: {testId: 5}}, + {typeName: 'withTestId', configuration: {testId: 6}}, + {typeName: 'withTestId', configuration: {testId: 7}} + ] + }, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 1, resolvedAt: null, + comments: [{id: 10, body: 'First comment', creatorName: 'Bob', creatorId: 2}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 2, resolvedAt: null, + comments: [{id: 11, body: 'Second comment', creatorName: 'Bob', creatorId: 2}]}, + {id: 3, subjectType: 'ContentElement', subjectId: 3, resolvedAt: null, + comments: [{id: 12, body: 'Third comment', creatorName: 'Bob', creatorId: 2}]} + ] + } + }); + } +}); diff --git a/entry_types/scrolled/package/spec/frontend/commenting/features/commentingBadges-spec.js b/entry_types/scrolled/package/spec/frontend/commenting/features/commentingBadges-spec.js index e31aaf1ec9..c075762a66 100644 --- a/entry_types/scrolled/package/spec/frontend/commenting/features/commentingBadges-spec.js +++ b/entry_types/scrolled/package/spec/frontend/commenting/features/commentingBadges-spec.js @@ -76,4 +76,52 @@ describe('commenting badges', () => { expect(entry.getByText('On the section')).toBeInTheDocument(); }); + + it('closes the thread list when clicking outside', () => { + const entry = renderEntry({ + seed: { + sections: [{id: 1, permaId: 10}], + contentElements: [{typeName: 'withTestId', configuration: {testId: 5}}] + }, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'Section', subjectId: 10, comments: [ + {id: 10, body: 'On the section', creatorName: 'Bob', creatorId: 2} + ]} + ] + } + }); + + fireEvent.click(entry.getAllCommentBadges()[0]); + expect(entry.getByText('On the section')).toBeInTheDocument(); + + fireEvent.mouseDown(document.body); + + expect(entry.queryByText('On the section')).not.toBeInTheDocument(); + }); + + it('closes the thread list when pressing Escape', () => { + const entry = renderEntry({ + seed: { + sections: [{id: 1, permaId: 10}], + contentElements: [{typeName: 'withTestId', configuration: {testId: 5}}] + }, + commenting: { + currentUser: {id: 42, name: 'Alice'}, + commentThreads: [ + {id: 1, subjectType: 'Section', subjectId: 10, comments: [ + {id: 10, body: 'On the section', creatorName: 'Bob', creatorId: 2} + ]} + ] + } + }); + + fireEvent.click(entry.getAllCommentBadges()[0]); + expect(entry.getByText('On the section')).toBeInTheDocument(); + + fireEvent.keyDown(document, {key: 'Escape'}); + + expect(entry.queryByText('On the section')).not.toBeInTheDocument(); + }); }); diff --git a/entry_types/scrolled/package/spec/frontend/useActiveExcursion-spec.js b/entry_types/scrolled/package/spec/frontend/useActiveExcursion-spec.js index 46a0026935..a35e0112d4 100644 --- a/entry_types/scrolled/package/spec/frontend/useActiveExcursion-spec.js +++ b/entry_types/scrolled/package/spec/frontend/useActiveExcursion-spec.js @@ -1,15 +1,24 @@ import {useActiveExcursion} from 'frontend/useActiveExcursion'; -import {useEntryStructure} from 'entryState'; import {renderHookInEntry} from 'support'; import {changeLocationHash} from 'support/changeLocationHash'; import {act} from '@testing-library/react'; -describe('useActiveExcursion', () => { - beforeEach(() => window.location.hash = '#initial'); +const mockScrollToTarget = jest.fn(); + +jest.mock('frontend/useScrollTarget', () => ({ + ...jest.requireActual('frontend/useScrollTarget'), + useScrollToTarget: () => mockScrollToTarget +})); + +describe('ActiveExcursionProvider', () => { + beforeEach(() => { + window.location.hash = '#initial'; + mockScrollToTarget.mockClear(); + }); it('returns undefined by default', () => { - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { } }); @@ -19,7 +28,7 @@ describe('useActiveExcursion', () => { }); it('returns excursion with slug matching hash', async () => { - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { storylines: [ {id: 1, configuration: {main: true}}, @@ -41,7 +50,7 @@ describe('useActiveExcursion', () => { }); it('returns undefined when hash matches main chapter slug', async () => { - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { storylines: [ {id: 1, configuration: {main: true}}, @@ -63,7 +72,7 @@ describe('useActiveExcursion', () => { }); it('supports resetting active excursion', async () => { - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { storylines: [ {id: 1, configuration: {main: true}}, @@ -90,7 +99,7 @@ describe('useActiveExcursion', () => { }); it('supports activating excursion via section id', async () => { - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { storylines: [ {id: 1, configuration: {main: true}}, @@ -120,7 +129,7 @@ describe('useActiveExcursion', () => { }); it('returns to initial hash after activating different excursions', async () => { - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { storylines: [ {id: 1, configuration: {main: true}}, @@ -155,7 +164,7 @@ describe('useActiveExcursion', () => { }); it('can return to initial empty hash', async () => { - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { storylines: [ {id: 1, configuration: {main: true}}, @@ -184,7 +193,7 @@ describe('useActiveExcursion', () => { }); it('activates excursion when hash matches section permaId pattern', async () => { - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { storylines: [ {id: 1, configuration: {main: true}}, @@ -210,7 +219,7 @@ describe('useActiveExcursion', () => { }); it('returns undefined when hash matches section permaId from main storyline', async () => { - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { storylines: [ {id: 1, configuration: {main: true}}, @@ -238,7 +247,7 @@ describe('useActiveExcursion', () => { it('activates excursion on initial render when hash matches chapter slug', () => { window.location.hash = '#excursion'; - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { storylines: [ {id: 1, configuration: {main: true}}, @@ -258,7 +267,7 @@ describe('useActiveExcursion', () => { it('activates excursion on initial render when hash matches section permaId pattern', () => { window.location.hash = '#section-501'; - const {result} = renderHookInEntry(() => useActiveExcursion(useEntryStructure()), { + const {result} = renderHookInEntry(() => useActiveExcursion(), { seed: { storylines: [ {id: 1, configuration: {main: true}}, @@ -284,273 +293,236 @@ describe('useActiveExcursion', () => { afterEach(() => jest.useRealTimers()); it('calls scrollToTarget when navigating from main to excursion section', async () => { - const scrollToTarget = jest.fn(); - - const {result} = renderHookInEntry( - () => useActiveExcursion(useEntryStructure(), {scrollToTarget}), - { - seed: { - storylines: [ - {id: 1, configuration: {main: true}}, - {id: 2} - ], - chapters: [ - {id: 10, storylineId: 1, configuration: {title: 'intro'}}, - {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, - ], - sections: [ - {id: 100, chapterId: 10, permaId: 500}, - {id: 101, chapterId: 11, permaId: 501}, - {id: 102, chapterId: 11, permaId: 502}, - ] - } + const {result} = renderHookInEntry(() => useActiveExcursion(), { + seed: { + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 10, storylineId: 1, configuration: {title: 'intro'}}, + {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, + ], + sections: [ + {id: 100, chapterId: 10, permaId: 500}, + {id: 101, chapterId: 11, permaId: 501}, + {id: 102, chapterId: 11, permaId: 502}, + ] } - ); + }); act(() => changeLocationHash('#section-502')); act(() => jest.advanceTimersByTime(500)); const {activeExcursion} = result.current; expect(activeExcursion).toMatchObject({title: 'excursion'}); - expect(scrollToTarget).toHaveBeenCalledWith({id: 102}); + expect(mockScrollToTarget).toHaveBeenCalledWith({id: 102}); }); it('calls scrollToTarget when navigating from excursion to main section', async () => { window.location.hash = '#excursion'; - const scrollToTarget = jest.fn(); - - const {result} = renderHookInEntry( - () => useActiveExcursion(useEntryStructure(), {scrollToTarget}), - { - seed: { - storylines: [ - {id: 1, configuration: {main: true}}, - {id: 2} - ], - chapters: [ - {id: 10, storylineId: 1, configuration: {title: 'intro'}}, - {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, - ], - sections: [ - {id: 100, chapterId: 10, permaId: 500}, - {id: 101, chapterId: 11, permaId: 501}, - ] - } + + const {result} = renderHookInEntry(() => useActiveExcursion(), { + seed: { + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 10, storylineId: 1, configuration: {title: 'intro'}}, + {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, + ], + sections: [ + {id: 100, chapterId: 10, permaId: 500}, + {id: 101, chapterId: 11, permaId: 501}, + ] } - ); + }); act(() => changeLocationHash('#section-500')); act(() => jest.advanceTimersByTime(500)); const {activeExcursion} = result.current; expect(activeExcursion).toBeUndefined(); - expect(scrollToTarget).toHaveBeenCalledWith({id: 100}); + expect(mockScrollToTarget).toHaveBeenCalledWith({id: 100}); }); it('calls scrollToTarget when navigating from excursion A to excursion B', async () => { window.location.hash = '#excursion1'; - const scrollToTarget = jest.fn(); - - const {result} = renderHookInEntry( - () => useActiveExcursion(useEntryStructure(), {scrollToTarget}), - { - seed: { - storylines: [ - {id: 1, configuration: {main: true}}, - {id: 2} - ], - chapters: [ - {id: 10, storylineId: 1, configuration: {title: 'intro'}}, - {id: 11, storylineId: 2, configuration: {title: 'excursion1'}}, - {id: 12, storylineId: 2, configuration: {title: 'excursion2'}} - ], - sections: [ - {id: 100, chapterId: 10, permaId: 500}, - {id: 101, chapterId: 11, permaId: 501}, - {id: 102, chapterId: 12, permaId: 502}, - {id: 103, chapterId: 12, permaId: 503} - ] - } + + const {result} = renderHookInEntry(() => useActiveExcursion(), { + seed: { + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 10, storylineId: 1, configuration: {title: 'intro'}}, + {id: 11, storylineId: 2, configuration: {title: 'excursion1'}}, + {id: 12, storylineId: 2, configuration: {title: 'excursion2'}} + ], + sections: [ + {id: 100, chapterId: 10, permaId: 500}, + {id: 101, chapterId: 11, permaId: 501}, + {id: 102, chapterId: 12, permaId: 502}, + {id: 103, chapterId: 12, permaId: 503} + ] } - ); + }); act(() => changeLocationHash('#section-503')); act(() => jest.advanceTimersByTime(500)); const {activeExcursion} = result.current; expect(activeExcursion).toMatchObject({title: 'excursion2'}); - expect(scrollToTarget).toHaveBeenCalledWith({id: 103}); + expect(mockScrollToTarget).toHaveBeenCalledWith({id: 103}); }); it('delays scroll by 500ms', async () => { - const scrollToTarget = jest.fn(); - - renderHookInEntry( - () => useActiveExcursion(useEntryStructure(), {scrollToTarget}), - { - seed: { - storylines: [ - {id: 1, configuration: {main: true}}, - {id: 2} - ], - chapters: [ - {id: 10, storylineId: 1, configuration: {title: 'intro'}}, - {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, - ], - sections: [ - {id: 100, chapterId: 10, permaId: 500}, - {id: 101, chapterId: 11, permaId: 501}, - {id: 102, chapterId: 11, permaId: 502}, - ] - } + renderHookInEntry(() => useActiveExcursion(), { + seed: { + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 10, storylineId: 1, configuration: {title: 'intro'}}, + {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, + ], + sections: [ + {id: 100, chapterId: 10, permaId: 500}, + {id: 101, chapterId: 11, permaId: 501}, + {id: 102, chapterId: 11, permaId: 502}, + ] } - ); + }); act(() => { changeLocationHash('#section-502'); }); - expect(scrollToTarget).not.toHaveBeenCalled(); + expect(mockScrollToTarget).not.toHaveBeenCalled(); act(() => { jest.advanceTimersByTime(499); }); - expect(scrollToTarget).not.toHaveBeenCalled(); + expect(mockScrollToTarget).not.toHaveBeenCalled(); act(() => { jest.advanceTimersByTime(1); }); - expect(scrollToTarget).toHaveBeenCalledWith({id: 102}); + expect(mockScrollToTarget).toHaveBeenCalledWith({id: 102}); }); it('does not call scrollToTarget when navigating within main storyline', async () => { - const scrollToTarget = jest.fn(); - - const {result} = renderHookInEntry( - () => useActiveExcursion(useEntryStructure(), {scrollToTarget}), - { - seed: { - storylines: [ - {id: 1, configuration: {main: true}}, - {id: 2} - ], - chapters: [ - {id: 10, storylineId: 1, configuration: {title: 'intro'}}, - {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, - ], - sections: [ - {id: 100, chapterId: 10, permaId: 500}, - {id: 103, chapterId: 10, permaId: 503}, - {id: 101, chapterId: 11, permaId: 501}, - ] - } + const {result} = renderHookInEntry(() => useActiveExcursion(), { + seed: { + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 10, storylineId: 1, configuration: {title: 'intro'}}, + {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, + ], + sections: [ + {id: 100, chapterId: 10, permaId: 500}, + {id: 103, chapterId: 10, permaId: 503}, + {id: 101, chapterId: 11, permaId: 501}, + ] } - ); + }); act(() => changeLocationHash('#section-503')); act(() => jest.advanceTimersByTime(500)); const {activeExcursion} = result.current; expect(activeExcursion).toBeUndefined(); - expect(scrollToTarget).not.toHaveBeenCalled(); + expect(mockScrollToTarget).not.toHaveBeenCalled(); }); it('does not call scrollToTarget when navigating within same excursion', async () => { window.location.hash = '#excursion'; - const scrollToTarget = jest.fn(); - - const {result} = renderHookInEntry( - () => useActiveExcursion(useEntryStructure(), {scrollToTarget}), - { - seed: { - storylines: [ - {id: 1, configuration: {main: true}}, - {id: 2} - ], - chapters: [ - {id: 10, storylineId: 1, configuration: {title: 'intro'}}, - {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, - ], - sections: [ - {id: 100, chapterId: 10, permaId: 500}, - {id: 101, chapterId: 11, permaId: 501}, - {id: 102, chapterId: 11, permaId: 502}, - ] - } + + const {result} = renderHookInEntry(() => useActiveExcursion(), { + seed: { + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 10, storylineId: 1, configuration: {title: 'intro'}}, + {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, + ], + sections: [ + {id: 100, chapterId: 10, permaId: 500}, + {id: 101, chapterId: 11, permaId: 501}, + {id: 102, chapterId: 11, permaId: 502}, + ] } - ); + }); act(() => changeLocationHash('#section-502')); act(() => jest.advanceTimersByTime(500)); const {activeExcursion} = result.current; expect(activeExcursion).toMatchObject({title: 'excursion'}); - expect(scrollToTarget).not.toHaveBeenCalled(); + expect(mockScrollToTarget).not.toHaveBeenCalled(); }); it('does not call scrollToTarget when hash does not match anything', async () => { - const scrollToTarget = jest.fn(); - - const {result} = renderHookInEntry( - () => useActiveExcursion(useEntryStructure(), {scrollToTarget}), - { - seed: { - storylines: [ - {id: 1, configuration: {main: true}}, - {id: 2} - ], - chapters: [ - {id: 10, storylineId: 1, configuration: {title: 'intro'}}, - {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, - ], - sections: [ - {id: 100, chapterId: 10, permaId: 500}, - {id: 101, chapterId: 11, permaId: 501}, - ] - } + const {result} = renderHookInEntry(() => useActiveExcursion(), { + seed: { + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 10, storylineId: 1, configuration: {title: 'intro'}}, + {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, + ], + sections: [ + {id: 100, chapterId: 10, permaId: 500}, + {id: 101, chapterId: 11, permaId: 501}, + ] } - ); + }); act(() => changeLocationHash('#nonexistent')); act(() => jest.advanceTimersByTime(500)); const {activeExcursion} = result.current; expect(activeExcursion).toBeUndefined(); - expect(scrollToTarget).not.toHaveBeenCalled(); + expect(mockScrollToTarget).not.toHaveBeenCalled(); }); it('scrolls again after returning from excursion and re-entering', async () => { - const scrollToTarget = jest.fn(); - - const {result} = renderHookInEntry( - () => useActiveExcursion(useEntryStructure(), {scrollToTarget}), - { - seed: { - storylines: [ - {id: 1, configuration: {main: true}}, - {id: 2} - ], - chapters: [ - {id: 10, storylineId: 1, configuration: {title: 'intro'}}, - {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, - ], - sections: [ - {id: 100, chapterId: 10, permaId: 500}, - {id: 101, chapterId: 11, permaId: 501}, - {id: 102, chapterId: 11, permaId: 502}, - ] - } + const {result} = renderHookInEntry(() => useActiveExcursion(), { + seed: { + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 10, storylineId: 1, configuration: {title: 'intro'}}, + {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, + ], + sections: [ + {id: 100, chapterId: 10, permaId: 500}, + {id: 101, chapterId: 11, permaId: 501}, + {id: 102, chapterId: 11, permaId: 502}, + ] } - ); + }); // Enter excursion (non-first section) act(() => changeLocationHash('#section-502')); act(() => jest.advanceTimersByTime(500)); - expect(scrollToTarget).toHaveBeenCalledWith({id: 102}); - scrollToTarget.mockClear(); + expect(mockScrollToTarget).toHaveBeenCalledWith({id: 102}); + mockScrollToTarget.mockClear(); // Return from excursion act(() => result.current.returnFromExcursion()); @@ -561,70 +533,60 @@ describe('useActiveExcursion', () => { act(() => changeLocationHash('#section-502')); act(() => jest.advanceTimersByTime(500)); - expect(scrollToTarget).toHaveBeenCalledWith({id: 102}); + expect(mockScrollToTarget).toHaveBeenCalledWith({id: 102}); }); it('does not scroll when navigating to excursion via slug', async () => { - const scrollToTarget = jest.fn(); - - const {result} = renderHookInEntry( - () => useActiveExcursion(useEntryStructure(), {scrollToTarget}), - { - seed: { - storylines: [ - {id: 1, configuration: {main: true}}, - {id: 2} - ], - chapters: [ - {id: 10, storylineId: 1, configuration: {title: 'intro'}}, - {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, - ], - sections: [ - {id: 100, chapterId: 10, permaId: 500}, - {id: 101, chapterId: 11, permaId: 501}, - ] - } + const {result} = renderHookInEntry(() => useActiveExcursion(), { + seed: { + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 10, storylineId: 1, configuration: {title: 'intro'}}, + {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, + ], + sections: [ + {id: 100, chapterId: 10, permaId: 500}, + {id: 101, chapterId: 11, permaId: 501}, + ] } - ); + }); act(() => changeLocationHash('#excursion')); act(() => jest.advanceTimersByTime(500)); const {activeExcursion} = result.current; expect(activeExcursion).toMatchObject({title: 'excursion'}); - expect(scrollToTarget).not.toHaveBeenCalled(); + expect(mockScrollToTarget).not.toHaveBeenCalled(); }); it('does not scroll when navigating to first section of excursion', async () => { - const scrollToTarget = jest.fn(); - - const {result} = renderHookInEntry( - () => useActiveExcursion(useEntryStructure(), {scrollToTarget}), - { - seed: { - storylines: [ - {id: 1, configuration: {main: true}}, - {id: 2} - ], - chapters: [ - {id: 10, storylineId: 1, configuration: {title: 'intro'}}, - {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, - ], - sections: [ - {id: 100, chapterId: 10, permaId: 500}, - {id: 101, chapterId: 11, permaId: 501}, - {id: 102, chapterId: 11, permaId: 502}, - ] - } + const {result} = renderHookInEntry(() => useActiveExcursion(), { + seed: { + storylines: [ + {id: 1, configuration: {main: true}}, + {id: 2} + ], + chapters: [ + {id: 10, storylineId: 1, configuration: {title: 'intro'}}, + {id: 11, storylineId: 2, configuration: {title: 'excursion'}}, + ], + sections: [ + {id: 100, chapterId: 10, permaId: 500}, + {id: 101, chapterId: 11, permaId: 501}, + {id: 102, chapterId: 11, permaId: 502}, + ] } - ); + }); act(() => changeLocationHash('#section-501')); act(() => jest.advanceTimersByTime(500)); const {activeExcursion} = result.current; expect(activeExcursion).toMatchObject({title: 'excursion'}); - expect(scrollToTarget).not.toHaveBeenCalled(); + expect(mockScrollToTarget).not.toHaveBeenCalled(); }); }); }); diff --git a/entry_types/scrolled/package/spec/review/ThreadList-spec.js b/entry_types/scrolled/package/spec/review/ThreadList-spec.js index 7268552a4f..9ba5b123fe 100644 --- a/entry_types/scrolled/package/spec/review/ThreadList-spec.js +++ b/entry_types/scrolled/package/spec/review/ThreadList-spec.js @@ -4,6 +4,7 @@ import userEvent from '@testing-library/user-event'; import {useFakeTranslations} from 'pageflow/testHelpers'; import {ThreadList} from 'review/ThreadList'; +import {ScrollHighlightedThreadIntoViewProvider} from 'review/scrollHighlightedThreadIntoView'; import {renderWithReviewState} from 'support/renderWithReviewState'; describe('ThreadList', () => { @@ -179,11 +180,13 @@ describe('ThreadList', () => { expect(onThreadClick).toHaveBeenCalledWith(expect.objectContaining({id: 7})); }); - it('scrolls the highlighted thread into view', () => { + it('scrolls the highlighted thread into view within the scroll context', () => { const {getByText} = renderWithReviewState( - , + + + , { commentThreads: [ {id: 1, subjectType: 'ContentElement', subjectId: 10, comments: [ @@ -202,6 +205,26 @@ describe('ThreadList', () => { .toBe(getByText('second').closest('[aria-current="true"]')); }); + it('does not scroll the highlighted thread into view outside the scroll context', () => { + renderWithReviewState( + , + { + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, comments: [ + {id: 10, body: 'first', creatorName: 'Alice', creatorId: 1} + ]}, + {id: 2, subjectType: 'ContentElement', subjectId: 10, comments: [ + {id: 20, body: 'second', creatorName: 'Bob', creatorId: 2} + ]} + ] + } + ); + + expect(Element.prototype.scrollIntoView).not.toHaveBeenCalled(); + }); + it('orders threads via compareRanges when provided', () => { const compareRanges = (a, b) => a.start - b.start; @@ -944,4 +967,55 @@ describe('ThreadList', () => { expect(getAllByPlaceholderText('Reply...')).toHaveLength(2); }); + + it('expands resolved threads by default when expandResolved is set', () => { + const {getByText} = renderWithReviewState( + , + { + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, + resolvedAt: null, + comments: [{id: 10, body: 'Active thread', creatorName: 'Alice', creatorId: 1}]}, + {id: 2, subjectType: 'ContentElement', subjectId: 10, + resolvedAt: '2026-04-09T10:00:00Z', + comments: [{id: 20, body: 'Resolved thread', creatorName: 'Bob', creatorId: 2}]} + ] + } + ); + + expect(getByText('Active thread')).toBeInTheDocument(); + expect(getByText('Resolved thread')).toBeInTheDocument(); + }); + + it('shows resolved threads instead of the new form when all are resolved and expandResolved is set', () => { + const {getByText, queryByPlaceholderText} = renderWithReviewState( + , + { + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, + resolvedAt: '2026-04-09T10:00:00Z', + comments: [{id: 10, body: 'Resolved thread', creatorName: 'Bob', creatorId: 2}]} + ] + } + ); + + expect(getByText('Resolved thread')).toBeInTheDocument(); + expect(queryByPlaceholderText('Add a comment...')).not.toBeInTheDocument(); + }); + + it('still auto-shows the new form for only-resolved threads without expandResolved', () => { + const {getByPlaceholderText, queryByText} = renderWithReviewState( + , + { + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, + resolvedAt: '2026-04-09T10:00:00Z', + comments: [{id: 10, body: 'Resolved thread', creatorName: 'Bob', creatorId: 2}]} + ] + } + ); + + expect(getByPlaceholderText('Add a comment...')).toBeInTheDocument(); + expect(queryByText('Resolved thread')).not.toBeInTheDocument(); + }); }); diff --git a/entry_types/scrolled/package/spec/review/ThreadsBadge-spec.js b/entry_types/scrolled/package/spec/review/ThreadsBadge-spec.js index f088df649b..c87050b838 100644 --- a/entry_types/scrolled/package/spec/review/ThreadsBadge-spec.js +++ b/entry_types/scrolled/package/spec/review/ThreadsBadge-spec.js @@ -3,6 +3,7 @@ import '@testing-library/jest-dom/extend-expect'; import {ThreadsBadge} from 'review/ThreadsBadge'; import {renderWithReviewState} from 'support/renderWithReviewState'; +import badgeStyles from 'review/Badge.module.css'; describe('ThreadsBadge', () => { it('does not display count for single thread', () => { @@ -61,6 +62,34 @@ describe('ThreadsBadge', () => { expect(container).toBeEmptyDOMElement(); }); + it('renders the badge as resolved when all shown threads are resolved', () => { + const {getByRole} = renderWithReviewState( + , + { + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, resolvedAt: '2026-04-09T10:00:00Z', comments: []}, + {id: 2, subjectType: 'ContentElement', subjectId: 10, resolvedAt: '2026-04-09T10:00:00Z', comments: []} + ] + } + ); + + expect(getByRole('status')).toHaveClass(badgeStyles.resolved); + }); + + it('does not render the badge as resolved when an unresolved thread remains', () => { + const {getByRole} = renderWithReviewState( + , + { + commentThreads: [ + {id: 1, subjectType: 'ContentElement', subjectId: 10, resolvedAt: '2026-04-09T10:00:00Z', comments: []}, + {id: 2, subjectType: 'ContentElement', subjectId: 10, resolvedAt: null, comments: []} + ] + } + ); + + expect(getByRole('status')).not.toHaveClass(badgeStyles.resolved); + }); + it('renders nothing when no threads exist for subject', () => { const {container} = renderWithReviewState( diff --git a/entry_types/scrolled/package/spec/review/useLocatedCommentThreads-spec.js b/entry_types/scrolled/package/spec/review/useLocatedCommentThreads-spec.js index 20130e3dba..828e268395 100644 --- a/entry_types/scrolled/package/spec/review/useLocatedCommentThreads-spec.js +++ b/entry_types/scrolled/package/spec/review/useLocatedCommentThreads-spec.js @@ -2,6 +2,7 @@ import React from 'react'; import {useLocatedCommentThreads} from 'review/useLocatedCommentThreads'; import {ReviewStateProvider} from 'review/ReviewStateProvider'; +import {review} from 'review/api'; import {renderHookInEntry} from 'support'; @@ -77,6 +78,24 @@ describe('useLocatedCommentThreads', () => { expect(result.current.chapters[1].threadCount).toBe(1); }); + it("orders a content element's threads by the type's compareRanges", () => { + review.contentElementTypes.register('textBlock', { + compareRanges: (a, b) => a - b + }); + + const {result} = renderLocatedCommentThreads([ + {id: 1, subjectType: 'ContentElement', subjectId: 10001, subjectRange: 2, comments: []}, + {id: 2, subjectType: 'ContentElement', subjectId: 10001, subjectRange: 1, comments: []} + ]); + + const element = result.current.chapters[0].sections[0].contentElements[0]; + + expect(element.threads.map(t => t.id)).toEqual([2, 1]); + expect(result.current.threads.map(t => t.id)).toEqual([2, 1]); + + review.contentElementTypes.types = {}; + }); + it('provides a flat list of threads in document order, section before its elements', () => { const {result} = renderLocatedCommentThreads([ {id: 1, subjectType: 'ContentElement', subjectId: 10001, comments: []}, diff --git a/entry_types/scrolled/package/spec/support/pageObjects/commenting.js b/entry_types/scrolled/package/spec/support/pageObjects/commenting.js index 95799971e6..2768ad6000 100644 --- a/entry_types/scrolled/package/spec/support/pageObjects/commenting.js +++ b/entry_types/scrolled/package/spec/support/pageObjects/commenting.js @@ -23,12 +23,17 @@ export function renderEntry({ return { ...result, + getCommentToolbar: () => result.getByRole('group', {name: 'Comments'}), getAddCommentButton: () => result.getByRole('button', {name: 'Add comment'}), getCancelAddCommentButton: () => result.getByRole('button', {name: 'Cancel add comment'}), getNewThreadInput: () => result.getByPlaceholderText('Add a comment...'), queryNewThreadInput: () => result.queryByPlaceholderText('Add a comment...'), getAllCommentBadges: () => result.getAllByRole('status'), - queryAllCommentBadges: () => result.queryAllByRole('status') + queryAllCommentBadges: () => result.queryAllByRole('status'), + getCommentFilterButton: resolution => + result.getByRole('button', {name: resolution === 'all' ? 'All' : 'Unresolved'}), + getPreviousCommentButton: () => result.getByRole('button', {name: 'Previous comment'}), + getNextCommentButton: () => result.getByRole('button', {name: 'Next comment'}) }; } @@ -44,9 +49,16 @@ export function useCommentingPageObjects() { useFakeTranslations({ 'pageflow_scrolled.review.add_comment': 'Add comment', 'pageflow_scrolled.review.cancel_add_comment': 'Cancel add comment', + 'pageflow_scrolled.review.comment_toolbar': 'Comments', + 'pageflow_scrolled.review.comment_count': '%{count} comments', 'pageflow_scrolled.review.select_content_element': 'Select to comment', 'pageflow_scrolled.review.select_section': 'Select section to comment', - 'pageflow_scrolled.review.add_comment_placeholder': 'Add a comment...' + 'pageflow_scrolled.review.add_comment_placeholder': 'Add a comment...', + 'pageflow_scrolled.review.filter.label': 'Filter comments', + 'pageflow_scrolled.review.filter.unresolved': 'Unresolved', + 'pageflow_scrolled.review.filter.all': 'All', + 'pageflow_scrolled.review.previous_comment': 'Previous comment', + 'pageflow_scrolled.review.next_comment': 'Next comment' }); usePageObjects(); diff --git a/entry_types/scrolled/package/src/contentElements/editor.js b/entry_types/scrolled/package/src/contentElements/editor.js index 6467ec3eb0..608f0c9057 100644 --- a/entry_types/scrolled/package/src/contentElements/editor.js +++ b/entry_types/scrolled/package/src/contentElements/editor.js @@ -19,3 +19,5 @@ import './question/editor' import './counter/editor' import './quote/editor' import './infoTable/editor' + +import './review'; diff --git a/entry_types/scrolled/package/src/contentElements/review.js b/entry_types/scrolled/package/src/contentElements/review.js new file mode 100644 index 0000000000..12792af7bd --- /dev/null +++ b/entry_types/scrolled/package/src/contentElements/review.js @@ -0,0 +1 @@ +import './textBlock/review'; diff --git a/entry_types/scrolled/package/src/contentElements/textBlock/editor.js b/entry_types/scrolled/package/src/contentElements/textBlock/editor.js index cb6381d8b1..3e3237d240 100644 --- a/entry_types/scrolled/package/src/contentElements/textBlock/editor.js +++ b/entry_types/scrolled/package/src/contentElements/textBlock/editor.js @@ -158,13 +158,6 @@ editor.contentElementTypes.register('textBlock', { return getValue(configuration).length; }, - compareRanges(a, b) { - if (!a && !b) return 0; - if (!a) return 1; - if (!b) return -1; - return Point.compare(rangeStart(a), rangeStart(b)); - }, - handleDestroy(contentElement) { const transientState = contentElement.get('transientState') || {}; @@ -202,10 +195,6 @@ function shiftPoint(point, delta) { return {...point, path: [point.path[0] + delta, ...point.path.slice(1)]}; } -function rangeStart(range) { - return Point.isBefore(range.anchor, range.focus) ? range.anchor : range.focus; -} - function endOfBlock(value, blockIndex) { const [lastNode, lastPath] = Node.last({children: value}, [blockIndex]); return {path: lastPath, offset: lastNode.text.length}; diff --git a/entry_types/scrolled/package/src/contentElements/textBlock/review.js b/entry_types/scrolled/package/src/contentElements/textBlock/review.js new file mode 100644 index 0000000000..02835221ca --- /dev/null +++ b/entry_types/scrolled/package/src/contentElements/textBlock/review.js @@ -0,0 +1,16 @@ +import {Point} from 'slate'; + +import {review} from 'pageflow-scrolled/review'; + +review.contentElementTypes.register('textBlock', { + compareRanges(a, b) { + if (!a && !b) return 0; + if (!a) return 1; + if (!b) return -1; + return Point.compare(rangeStart(a), rangeStart(b)); + } +}); + +function rangeStart(range) { + return Point.isBefore(range.anchor, range.focus) ? range.anchor : range.focus; +} diff --git a/entry_types/scrolled/package/src/editor/api/ContentElementTypeRegistry.js b/entry_types/scrolled/package/src/editor/api/ContentElementTypeRegistry.js index 2c4c5c06b0..811d4b4f98 100644 --- a/entry_types/scrolled/package/src/editor/api/ContentElementTypeRegistry.js +++ b/entry_types/scrolled/package/src/editor/api/ContentElementTypeRegistry.js @@ -83,10 +83,6 @@ export class ContentElementTypeRegistry { return this.contentElementTypes[typeName]?.pictogram; } - findCompareRanges(typeName) { - return this.contentElementTypes[typeName]?.compareRanges; - } - groupedByCategory() { const result = []; const categoriesByName = {}; diff --git a/entry_types/scrolled/package/src/editor/views/EntryCommentsView.js b/entry_types/scrolled/package/src/editor/views/EntryCommentsView.js index 0b5b16f1cd..b5483808c8 100644 --- a/entry_types/scrolled/package/src/editor/views/EntryCommentsView.js +++ b/entry_types/scrolled/package/src/editor/views/EntryCommentsView.js @@ -2,7 +2,7 @@ import React, {useEffect} from 'react'; import I18n from 'i18n-js'; import {EntryStateProvider, useEntryStateDispatch, watchCollections} from 'pageflow-scrolled/entryState'; -import {ThreadList, useLocatedCommentThreads} from 'pageflow-scrolled/review'; +import {ThreadList, useLocatedCommentThreads, review} from 'pageflow-scrolled/review'; import {ReviewView} from './ReviewView'; import defaultPictogram from './images/defaultPictogram.svg'; @@ -148,7 +148,7 @@ function ContentElementGroup({ const {permaId, type, threads} = contentElement; const label = I18n.t(`pageflow_scrolled.editor.content_elements.${type}.name`); const pictogram = editor.contentElementTypes.findPictogram(type) || defaultPictogram; - const compareRanges = editor.contentElementTypes.findCompareRanges(type); + const compareRanges = review.contentElementTypes.findCompareRanges(type); const isSelected = selectedSubject?.subjectType === 'ContentElement' && selectedSubject.id === contentElement.id; diff --git a/entry_types/scrolled/package/src/editor/views/ReviewView.js b/entry_types/scrolled/package/src/editor/views/ReviewView.js index b8f64c4a5f..dc5f74d630 100644 --- a/entry_types/scrolled/package/src/editor/views/ReviewView.js +++ b/entry_types/scrolled/package/src/editor/views/ReviewView.js @@ -2,7 +2,11 @@ import React from 'react'; import ReactDOM from 'react-dom'; import Marionette from 'backbone.marionette'; -import {ReviewStateProvider, ReviewMessageHandler} from 'pageflow-scrolled/review'; +import { + ReviewStateProvider, + ReviewMessageHandler, + ScrollHighlightedThreadIntoViewProvider +} from 'pageflow-scrolled/review'; import styles from './ReviewView.module.css'; @@ -41,7 +45,9 @@ export const ReviewView = Marionette.ItemView.extend({ const session = this.options.entry.reviewSession; ReactDOM.render( - {this.renderContent(this.props())} + + {this.renderContent(this.props())} + , this._containerEl() ); diff --git a/entry_types/scrolled/package/src/editor/views/SelectionCommentsView.js b/entry_types/scrolled/package/src/editor/views/SelectionCommentsView.js index c2a367055b..07a1ab1c68 100644 --- a/entry_types/scrolled/package/src/editor/views/SelectionCommentsView.js +++ b/entry_types/scrolled/package/src/editor/views/SelectionCommentsView.js @@ -1,6 +1,6 @@ import React from 'react'; -import {ThreadList} from 'pageflow-scrolled/review'; +import {ThreadList, review} from 'pageflow-scrolled/review'; import {ReviewView} from './ReviewView'; import styles from './SelectionCommentsView.module.css'; @@ -21,7 +21,7 @@ export const SelectionCommentsView = ReviewView.extend({ }, props() { - const {entry, editor} = this.options; + const {entry} = this.options; const subject = entry.get('selectedCommentsSubject'); // Resolve the model from the current subject rather than the cached @@ -41,7 +41,7 @@ export const SelectionCommentsView = ReviewView.extend({ subjectType: 'ContentElement', subjectId: model.get('permaId'), threadIds: model.transientState.get('commentThreadIdsAtSelection'), - compareRanges: typeName && editor.contentElementTypes.findCompareRanges(typeName), + compareRanges: typeName && review.contentElementTypes.findCompareRanges(typeName), highlightedThreadId: entry.get('highlightedThreadId'), onThreadClick: thread => entry.trigger('selectCommentThread', thread.id) }; diff --git a/entry_types/scrolled/package/src/frontend/Content.js b/entry_types/scrolled/package/src/frontend/Content.js index b9434433cc..a692cb6d87 100644 --- a/entry_types/scrolled/package/src/frontend/Content.js +++ b/entry_types/scrolled/package/src/frontend/Content.js @@ -26,11 +26,7 @@ export const Content = extensible('Content', function Content(props) { const entryStructure = useEntryStructure(); const scrollToTarget = useScrollToTarget(); - const { - activeExcursion, - activateExcursionOfSection, - returnFromExcursion - } = useActiveExcursion(entryStructure, {scrollToTarget}); + const {activeExcursion, activateExcursionOfSection, returnFromExcursion} = useActiveExcursion(); const [currentSectionIndex, setCurrentSectionIndexState] = useCurrentSectionIndexState(); const [currentExcursionSectionIndex, setCurrentExcursionSectionIndex] = useState(0); diff --git a/entry_types/scrolled/package/src/frontend/RootProviders.js b/entry_types/scrolled/package/src/frontend/RootProviders.js index b4dfc0fbab..7e014720f1 100644 --- a/entry_types/scrolled/package/src/frontend/RootProviders.js +++ b/entry_types/scrolled/package/src/frontend/RootProviders.js @@ -14,6 +14,7 @@ import {ConsentProvider} from './thirdPartyConsent'; import {CurrentSectionProvider} from './useCurrentChapter'; import {ExtensionsProvider} from './extensionRegistry'; import {ScrollTargetEmitterProvider} from './useScrollTarget'; +import {ActiveExcursionProvider} from './useActiveExcursion'; export function RootProviders({seed, consent = consentApi, children}) { return ( @@ -29,7 +30,9 @@ export function RootProviders({seed, consent = consentApi, children}) { - {children} + + {children} + diff --git a/entry_types/scrolled/package/src/frontend/commenting/CommentDisplayFilterProvider.js b/entry_types/scrolled/package/src/frontend/commenting/CommentDisplayFilterProvider.js new file mode 100644 index 0000000000..1faeb73176 --- /dev/null +++ b/entry_types/scrolled/package/src/frontend/commenting/CommentDisplayFilterProvider.js @@ -0,0 +1,22 @@ +import React, {createContext, useContext, useMemo, useState} from 'react'; + +const CommentDisplayFilterContext = createContext({ + resolution: 'unresolved', + setResolution: () => {} +}); + +export function CommentDisplayFilterProvider({children}) { + const [resolution, setResolution] = useState('unresolved'); + + const value = useMemo(() => ({resolution, setResolution}), [resolution]); + + return ( + + {children} + + ); +} + +export function useCommentDisplayFilter() { + return useContext(CommentDisplayFilterContext); +} diff --git a/entry_types/scrolled/package/src/frontend/commenting/EditableText.js b/entry_types/scrolled/package/src/frontend/commenting/EditableText.js index c36930e432..d8d07e27c4 100644 --- a/entry_types/scrolled/package/src/frontend/commenting/EditableText.js +++ b/entry_types/scrolled/package/src/frontend/commenting/EditableText.js @@ -8,6 +8,7 @@ import {useCommentThreads, useCommentHighlights, decorateCommentHighlights, useR import {PlainEditableText, renderElement, renderLeaf} from '../EditableText'; import {useContentElementAttributes} from '../useContentElementAttributes'; import {useAddCommentMode} from './AddCommentModeProvider'; +import {useCommentDisplayFilter} from './CommentDisplayFilterProvider'; import {useSelectedSubject} from './SelectedSubjectProvider'; import {AddCommentHint} from './AddCommentHint'; import {PopoversColumn} from './PopoversColumn'; @@ -40,10 +41,11 @@ function CommentingEditableText({ const {contentElementPermaId} = useContentElementAttributes(); const {active, deactivate, preselect, clearPreselection} = useAddCommentMode(); const {subjectRange, select} = useSelectedSubject('ContentElement', contentElementPermaId); + const {resolution} = useCommentDisplayFilter(); const threads = useCommentThreads({ subjectType: 'ContentElement', subjectId: contentElementPermaId, - resolution: 'unresolved' + resolution }); const highlights = useCommentHighlights(threads, subjectRange); diff --git a/entry_types/scrolled/package/src/frontend/commenting/EntryDecorator.js b/entry_types/scrolled/package/src/frontend/commenting/EntryDecorator.js index a75ba008a4..3341a4fdd8 100644 --- a/entry_types/scrolled/package/src/frontend/commenting/EntryDecorator.js +++ b/entry_types/scrolled/package/src/frontend/commenting/EntryDecorator.js @@ -4,6 +4,7 @@ import {useEntryMetadata} from 'pageflow-scrolled/entryState'; import {createReviewSession} from 'pageflow/review'; import {ReviewStateProvider, ReviewMessageHandler} from 'pageflow-scrolled/review'; import {AddCommentModeProvider} from './AddCommentModeProvider'; +import {CommentDisplayFilterProvider} from './CommentDisplayFilterProvider'; import {SelectedSubjectProvider} from './SelectedSubjectProvider'; import {FloatingToolbar} from './FloatingToolbar'; @@ -11,12 +12,14 @@ export function EntryDecorator({commentingInitialState, children}) { return ( - - - {children} - - - + + + + {children} + + + + ); } diff --git a/entry_types/scrolled/package/src/frontend/commenting/FloatingToolbar.js b/entry_types/scrolled/package/src/frontend/commenting/FloatingToolbar.js index 87b76743e6..2886249e77 100644 --- a/entry_types/scrolled/package/src/frontend/commenting/FloatingToolbar.js +++ b/entry_types/scrolled/package/src/frontend/commenting/FloatingToolbar.js @@ -1,14 +1,103 @@ import React from 'react'; +import classNames from 'classnames'; +import {useLocatedCommentThreads} from 'pageflow-scrolled/review'; import {useI18n} from '../i18n'; import {useAddCommentMode} from './AddCommentModeProvider'; +import {useCommentDisplayFilter} from './CommentDisplayFilterProvider'; +import {useCommentNavigation} from './SelectedSubjectProvider'; import AddCommentIcon from './images/addComment.svg'; import CancelCommentIcon from './images/cancelComment.svg'; +import ChevronIcon from './images/chevron.svg'; import styles from './FloatingToolbar.module.css'; export function FloatingToolbar() { const {t} = useI18n({locale: 'ui'}); + + return ( +
+ + + + +
+ ); +} + +function PositionIndicator() { + const {t} = useI18n({locale: 'ui'}); + const {count, position} = useCommentNavigation(); + + return ( + + {position || '–'} / + + ); +} + +function NavigationArrows() { + const {t} = useI18n({locale: 'ui'}); + const {count, goToPrevious, goToNext} = useCommentNavigation(); + + return ( +
+ + +
+ ); +} + +const resolutions = ['unresolved', 'all']; + +function ResolutionToggleButton() { + const {t} = useI18n({locale: 'ui'}); + const {resolution, setResolution} = useCommentDisplayFilter(); + const {threads} = useLocatedCommentThreads(); + + const counts = { + unresolved: threads.filter(thread => !thread.resolvedAt).length, + all: threads.length + }; + + return ( +
+ {resolutions.map(value => ( + + ))} +
+ ); +} + +function AddCommentButton() { + const {t} = useI18n({locale: 'ui'}); const {active, toggle} = useAddCommentMode(); const Icon = active ? CancelCommentIcon : AddCommentIcon; @@ -17,14 +106,12 @@ export function FloatingToolbar() { : 'pageflow_scrolled.review.add_comment'); return ( -
- -
+ ); } diff --git a/entry_types/scrolled/package/src/frontend/commenting/FloatingToolbar.module.css b/entry_types/scrolled/package/src/frontend/commenting/FloatingToolbar.module.css index cf63497b7c..adfcccc6c0 100644 --- a/entry_types/scrolled/package/src/frontend/commenting/FloatingToolbar.module.css +++ b/entry_types/scrolled/package/src/frontend/commenting/FloatingToolbar.module.css @@ -1,20 +1,114 @@ .toolbar { position: fixed; - bottom: 0; - right: 0; + bottom: space(3); + right: space(3); z-index: 1100; - padding: space(2); + display: flex; + align-items: center; + gap: space(2); + padding: space(1); + background: var(--ui-primary-color); + border: 1px solid var(--ui-on-primary-color-lightest); + border-radius: rounded(lg); + box-shadow: var(--ui-box-shadow-md); +} + +.navigation { + display: flex; + align-items: center; + gap: space(1); } .button { font-family: var(--ui-font-family); color: var(--ui-on-primary-color); - background: var(--ui-primary-color); + background: transparent; border: none; - border-radius: rounded(md); - padding: space(2); + border-radius: rounded(sm); + padding: space(1); cursor: pointer; - box-shadow: var(--ui-box-shadow-md); display: flex; align-items: center; } + +.button:disabled { + opacity: 0.3; + cursor: default; +} + +.count, +.segmented { + font-size: size(3.5); +} + +.count { + font-family: var(--ui-font-family); + font-variant-numeric: tabular-nums; + color: var(--ui-on-primary-color-light); + padding: 0 space(1); + margin: 0 space(-1) 0 space(2); + white-space: nowrap; +} + +.chevronUp { + transform: rotate(180deg); +} + +.chevronDown { + transform: rotate(0deg); +} + +.segmented { + display: flex; + align-items: center; + gap: space(1); + border: solid 1px var(--ui-on-primary-color-lightest); + border-radius: rounded(md); +} + +.segment { + font-family: var(--ui-font-family); + display: flex; + align-items: center; + gap: space(1); + color: var(--ui-on-primary-color-light); + background: transparent; + border: none; + padding: space(2) space(2); + cursor: pointer; + white-space: nowrap; +} + +.segment:first-child { + border-top-left-radius: rounded(md); + border-bottom-left-radius: rounded(md); +} + +.segment:last-child { + border-top-right-radius: rounded(md); + border-bottom-right-radius: rounded(md); +} + +.segment[aria-pressed='true'] { + color: var(--ui-on-primary-color); + background: var(--ui-on-primary-color-lightest); +} + +.segmentCount { + font-variant-numeric: tabular-nums; + border-radius: rounded(sm); + padding: 0 space(1); + background: var(--ui-on-primary-color-lightest); +} + +.segment[aria-pressed='true'] .segmentCount { + background: var(--ui-on-primary-color-lighter); +} + +.addButton { + color: var(--ui-on-accent-color); + background: var(--ui-accent-color); + border-radius: rounded(md); + padding: space(2); + margin-left: space(2); +} diff --git a/entry_types/scrolled/package/src/frontend/commenting/Popover.js b/entry_types/scrolled/package/src/frontend/commenting/Popover.js index 9b503420cb..fec3bae330 100644 --- a/entry_types/scrolled/package/src/frontend/commenting/Popover.js +++ b/entry_types/scrolled/package/src/frontend/commenting/Popover.js @@ -1,4 +1,4 @@ -import React, {useEffect} from 'react'; +import React, {useEffect, useState} from 'react'; import { useFloating, FloatingPortal, offset, flip, shift, autoUpdate @@ -6,6 +6,7 @@ import { import {ThreadsBadge, ThreadList} from 'pageflow-scrolled/review'; import {useFloatingPortalRoot} from '../FloatingPortalRootProvider'; +import {useCommentDisplayFilter} from './CommentDisplayFilterProvider'; import {useSelectedSubject} from './SelectedSubjectProvider'; import styles from './Popover.module.css'; @@ -14,12 +15,65 @@ export function Popover({ subjectType, subjectId, subjectRange, placement = 'bottom-start', strategy = 'absolute', hideNewTopicButton }) { - const {isSelected, showNewForm, select, clearSelection} = + const {isSelected, showNewForm, select, clearSelection, highlightedThreadId} = useSelectedSubject(subjectType, subjectId, subjectRange); + const {resolution} = useCommentDisplayFilter(); + const [reference, setReference] = useState(null); + + // Scroll into view when navigation lands on this subject. Only + // navigation sets a highlighted thread (clicking a badge does not), so + // this never fires on plain selection. Re-firing while stepping + // between a subject's threads targets the same badge, so it is a + // no-op; a popover mounting late (after its excursion is activated) + // scrolls once its reference becomes available. + useEffect(() => { + if (isSelected && highlightedThreadId != null) { + reference?.scrollIntoView({block: 'center', behavior: 'smooth'}); + } + }, [isSelected, highlightedThreadId, reference]); + + function handleBadgeClick() { + if (isSelected) { + clearSelection(); + } + else { + select(); + } + } + + return ( + + + {isSelected && + } + + ); +} + +function OpenThreadList({ + reference, subjectType, subjectId, subjectRange, + placement, strategy, showNewForm, hideNewTopicButton, highlightedThreadId, expandResolved, onDismiss +}) { const portalRoot = useFloatingPortalRoot(); const {refs, floatingStyles} = useFloating({ - open: isSelected, + open: true, + elements: {reference}, placement, strategy, middleware: [ @@ -33,29 +87,19 @@ export function Popover({ whileElementsMounted: autoUpdate }); - function handleBadgeClick() { - if (isSelected) { - clearSelection(); - } - else { - select(); - } - } - useEffect(() => { - if (!isSelected) return; - function handleClick(event) { - if (refs.reference.current?.contains(event.target)) return; + if (reference?.contains(event.target)) return; if (refs.floating.current?.contains(event.target)) return; if (event.target.closest('[data-comment-highlight]')) return; + if (event.target.closest('[data-comment-toolbar]')) return; - clearSelection(); + onDismiss(); } function handleKeyDown(event) { if (event.key === 'Escape') { - clearSelection(); + onDismiss(); } } @@ -66,28 +110,22 @@ export function Popover({ document.removeEventListener('mousedown', handleClick); document.removeEventListener('keydown', handleKeyDown); }; - }, [isSelected, clearSelection, refs.reference, refs.floating]); + }, [reference, refs.floating, onDismiss]); return ( - - +
+ - {isSelected && - -
- -
- } - + highlightedThreadId={highlightedThreadId} + expandResolved={expandResolved} + showNewForm={showNewForm} + hideNewTopicButton={hideNewTopicButton} /> +
+ ); } diff --git a/entry_types/scrolled/package/src/frontend/commenting/SectionDecorator.js b/entry_types/scrolled/package/src/frontend/commenting/SectionDecorator.js index b0688d0115..64da981d2b 100644 --- a/entry_types/scrolled/package/src/frontend/commenting/SectionDecorator.js +++ b/entry_types/scrolled/package/src/frontend/commenting/SectionDecorator.js @@ -4,6 +4,7 @@ import classNames from 'classnames'; import {useCommentThreads} from 'pageflow-scrolled/review'; import {useI18n} from '../i18n'; import {useAddCommentMode} from './AddCommentModeProvider'; +import {useCommentDisplayFilter} from './CommentDisplayFilterProvider'; import {useSelectedSubject} from './SelectedSubjectProvider'; import {Popover} from './Popover'; @@ -13,10 +14,11 @@ import styles from './SectionDecorator.module.css'; export function SectionDecorator({section, children}) { const {active} = useAddCommentMode(); const {isSelected} = useSelectedSubject('Section', section.permaId); + const {resolution} = useCommentDisplayFilter(); const threads = useCommentThreads({ subjectType: 'Section', subjectId: section.permaId, - resolution: 'unresolved' + resolution }); const hasThreads = threads.length > 0; diff --git a/entry_types/scrolled/package/src/frontend/commenting/SelectedSubjectProvider.js b/entry_types/scrolled/package/src/frontend/commenting/SelectedSubjectProvider.js index 9f509ef91e..7a87719b7a 100644 --- a/entry_types/scrolled/package/src/frontend/commenting/SelectedSubjectProvider.js +++ b/entry_types/scrolled/package/src/frontend/commenting/SelectedSubjectProvider.js @@ -1,31 +1,101 @@ import React, {createContext, useCallback, useContext, useMemo, useState} from 'react'; +import {useLocatedCommentThreads} from 'pageflow-scrolled/review'; +import {useActiveExcursion} from '../useActiveExcursion'; +import {useCommentDisplayFilter} from './CommentDisplayFilterProvider'; + const SelectedSubjectContext = createContext({ selectedSubject: null, setSelectedSubject: () => {}, clearSelection: () => {} }); +const CommentNavigationContext = createContext({ + count: 0, + position: 0, + goToNext: () => {}, + goToPrevious: () => {} +}); + export function SelectedSubjectProvider({children}) { + const {resolution} = useCommentDisplayFilter(); + const {chapters} = useLocatedCommentThreads(); + const {activateExcursionOfSection, returnFromExcursion} = useActiveExcursion(); + const [selectedSubject, setSelectedSubject] = useState(null); + const targets = useMemo( + () => navigableTargets(chapters, resolution), + [chapters, resolution] + ); + const clearSelection = useCallback(() => { setSelectedSubject(null); }, []); - const value = useMemo(() => ({ + const goTo = useCallback(step => { + if (targets.length === 0) { + return; + } + + const current = currentTargetIndex(targets, selectedSubject); + const next = current < 0 + ? (step > 0 ? 0 : targets.length - 1) + : (current + step + targets.length) % targets.length; + + const target = targets[next]; + + // Activate the excursion the target lives in (or leave the current + // one) before selecting it, so its popover can mount and open. Only + // needed when moving to a different subject. + if (!selectedSubject || subjectKey(selectedSubject) !== target.key) { + if (target.excursion) { + activateExcursionOfSection({id: target.sectionId}); + } + else { + returnFromExcursion(); + } + } + + setSelectedSubject({ + subjectType: target.subjectType, + subjectId: target.subjectId, + subjectRange: target.subjectRange, + highlightedThreadId: target.threadId + }); + }, [targets, selectedSubject, activateExcursionOfSection, returnFromExcursion]); + + const position = useMemo( + () => currentTargetIndex(targets, selectedSubject) + 1, + [targets, selectedSubject] + ); + + const selection = useMemo(() => ({ selectedSubject, setSelectedSubject, clearSelection }), [selectedSubject, clearSelection]); + const navigation = useMemo(() => ({ + count: targets.length, + position, + goToNext: () => goTo(1), + goToPrevious: () => goTo(-1) + }), [targets.length, position, goTo]); + return ( - - {children} + + + {children} + ); } +export function useCommentNavigation() { + return useContext(CommentNavigationContext); +} + export function useSelectedSubject(subjectType, subjectId, subjectRange) { const {selectedSubject, setSelectedSubject, clearSelection} = useContext(SelectedSubjectContext); @@ -40,5 +110,61 @@ export function useSelectedSubject(subjectType, subjectId, subjectRange) { return {isSelected, hasSelection: !!selectedSubject, select, clearSelection, showNewForm: isSelected && selectedSubject.showNewForm, - subjectRange: isSelected ? selectedSubject.subjectRange : undefined}; + subjectRange: isSelected ? selectedSubject.subjectRange : undefined, + highlightedThreadId: isSelected ? selectedSubject.highlightedThreadId ?? null : null}; +} + +// The cursor the arrows step from: the highlighted thread when one is +// set by navigation, otherwise the selected subject's first thread (so +// arrows continue from a clicked badge), otherwise nothing. +function currentTargetIndex(targets, selectedSubject) { + if (!selectedSubject) { + return -1; + } + + if (selectedSubject.highlightedThreadId != null) { + return targets.findIndex(target => target.threadId === selectedSubject.highlightedThreadId); + } + + const key = subjectKey(selectedSubject); + return targets.findIndex(target => target.key === key); +} + +function navigableTargets(chapters, resolution) { + const targets = []; + + chapters.forEach(chapter => { + chapter.sections.forEach(section => { + const location = {sectionId: section.id, excursion: chapter.isExcursion}; + + pushTargets(targets, section.threads, location); + section.contentElements.forEach(contentElement => { + pushTargets(targets, contentElement.threads, location); + }); + }); + }); + + return targets.filter(target => matchesResolution(target, resolution)); +} + +function pushTargets(targets, threads, location) { + threads.forEach(thread => targets.push({ + key: subjectKey(thread), + subjectType: thread.subjectType, + subjectId: thread.subjectId, + subjectRange: thread.subjectRange, + threadId: thread.id, + resolved: !!thread.resolvedAt, + ...location + })); +} + +function matchesResolution(target, resolution) { + return resolution === 'all' || + (resolution === 'unresolved' && !target.resolved) || + (resolution === 'resolved' && target.resolved); +} + +function subjectKey({subjectType, subjectId, subjectRange}) { + return `${subjectType}:${subjectId}:${subjectRange ? JSON.stringify(subjectRange) : ''}`; } diff --git a/entry_types/scrolled/package/src/frontend/commenting/extensions.js b/entry_types/scrolled/package/src/frontend/commenting/extensions.js index da73844007..f06cbd32f4 100644 --- a/entry_types/scrolled/package/src/frontend/commenting/extensions.js +++ b/entry_types/scrolled/package/src/frontend/commenting/extensions.js @@ -1,3 +1,5 @@ +import '../../contentElements/review'; + import {EntryDecorator} from './EntryDecorator'; import {SectionDecorator} from './SectionDecorator'; import {ContentElementDecorator} from './ContentElementDecorator'; diff --git a/entry_types/scrolled/package/src/frontend/commenting/images/chevron.svg b/entry_types/scrolled/package/src/frontend/commenting/images/chevron.svg new file mode 100644 index 0000000000..a96f8096bc --- /dev/null +++ b/entry_types/scrolled/package/src/frontend/commenting/images/chevron.svg @@ -0,0 +1 @@ + diff --git a/entry_types/scrolled/package/src/frontend/useActiveExcursion.js b/entry_types/scrolled/package/src/frontend/useActiveExcursion.js index c65a2f1ce2..3fddee1a2d 100644 --- a/entry_types/scrolled/package/src/frontend/useActiveExcursion.js +++ b/entry_types/scrolled/package/src/frontend/useActiveExcursion.js @@ -1,6 +1,18 @@ -import {useCallback, useEffect, useState, useMemo, useRef} from 'react'; +import React, {createContext, useCallback, useContext, useEffect, useState, useMemo, useRef} from 'react'; + +import {useEntryStructure} from 'pageflow-scrolled/entryState'; +import {useScrollToTarget} from './useScrollTarget'; + +const ActiveExcursionContext = createContext({ + activeExcursion: undefined, + activateExcursionOfSection: () => {}, + returnFromExcursion: () => {} +}); + +export function ActiveExcursionProvider({children}) { + const entryStructure = useEntryStructure(); + const scrollToTarget = useScrollToTarget(); -export function useActiveExcursion(entryStructure, {scrollToTarget} = {}) { const [activeExcursionId, setActiveExcursionId] = useState(); const [scrollTarget, setScrollTarget] = useState(); const returnUrlRef = useRef(null); @@ -109,7 +121,20 @@ export function useActiveExcursion(entryStructure, {scrollToTarget} = {}) { const activeExcursion = useMemo(() => { return entryStructure.excursions.find(excursion => excursion.id === activeExcursionId); - }, [entryStructure, activeExcursionId]) + }, [entryStructure, activeExcursionId]); + + const value = useMemo( + () => ({activeExcursion, activateExcursionOfSection, returnFromExcursion}), + [activeExcursion, activateExcursionOfSection, returnFromExcursion] + ); + + return ( + + {children} + + ); +} - return {activeExcursion, activateExcursionOfSection, returnFromExcursion}; +export function useActiveExcursion() { + return useContext(ActiveExcursionContext); } diff --git a/entry_types/scrolled/package/src/review/Thread.js b/entry_types/scrolled/package/src/review/Thread.js index c06bd5a68c..5527fcf0ab 100644 --- a/entry_types/scrolled/package/src/review/Thread.js +++ b/entry_types/scrolled/package/src/review/Thread.js @@ -5,6 +5,7 @@ import {useI18n} from '../frontend/i18n'; import {AvatarStack} from './Avatar'; import {Comment} from './Comment'; import {ReplyForm} from './ReplyForm'; +import {useScrollHighlightedThreadIntoView} from './scrollHighlightedThreadIntoView'; import ChevronIcon from './images/chevron.svg'; import ResolveIcon from './images/resolve.svg'; @@ -18,12 +19,13 @@ export function Thread({thread, collapsed, onToggle, onResolve, onClick, highlig const repliesCollapsed = collapsed && replies.length > 0; const ref = useRef(); + const scrollHighlightedIntoView = useScrollHighlightedThreadIntoView(); useEffect(() => { - if (highlighted && ref.current) { + if (scrollHighlightedIntoView && highlighted && ref.current) { ref.current.scrollIntoView({block: 'nearest', behavior: 'smooth'}); } - }, [highlighted]); + }, [scrollHighlightedIntoView, highlighted]); return (
{!showNewForm && !hideNewTopicButton && @@ -78,7 +81,7 @@ export function ThreadList({subjectType, subjectId, subjectRange, filter, compar {resolvedThreads.length > 0 &&
); } - -function sortByRange(threads, compareRanges) { - if (!compareRanges) return threads; - return [...threads].sort((a, b) => compareRanges(a.subjectRange, b.subjectRange)); -} diff --git a/entry_types/scrolled/package/src/review/ThreadsBadge.js b/entry_types/scrolled/package/src/review/ThreadsBadge.js index 6179cfa013..754e958e6e 100644 --- a/entry_types/scrolled/package/src/review/ThreadsBadge.js +++ b/entry_types/scrolled/package/src/review/ThreadsBadge.js @@ -3,12 +3,15 @@ import React, {useCallback} from 'react'; import {useCommentThreads} from './ReviewStateProvider'; import {Badge} from './Badge'; -export function ThreadsBadge({subjectType, subjectId, subjectRange, onClick, mode}) { - const threads = useCommentThreads({subjectType, subjectId, subjectRange, resolution: 'unresolved'}); +export function ThreadsBadge({subjectType, subjectId, subjectRange, onClick, mode, resolution = 'unresolved'}) { + const threads = useCommentThreads({subjectType, subjectId, subjectRange, resolution}); + const unresolvedThreads = useCommentThreads({subjectType, subjectId, subjectRange, resolution: 'unresolved'}); const handleClick = useCallback(() => { if (onClick) onClick(threads); }, [onClick, threads]); - return ; + const resolved = threads.length > 0 && unresolvedThreads.length === 0; + + return ; } diff --git a/entry_types/scrolled/package/src/review/api/ContentElementTypeRegistry.js b/entry_types/scrolled/package/src/review/api/ContentElementTypeRegistry.js new file mode 100644 index 0000000000..b589e9fb39 --- /dev/null +++ b/entry_types/scrolled/package/src/review/api/ContentElementTypeRegistry.js @@ -0,0 +1,18 @@ +// Holds per content element type behavior needed to display comment +// threads. Available both in the editor and in the frontend commenting +// mode. Kept separate from the editor and frontend registries so comment +// ordering logic (comparing Slate ranges) can be shared by both without +// pulling Slate into the always-on frontend bundle. +export class ContentElementTypeRegistry { + constructor() { + this.types = {}; + } + + register(typeName, options) { + this.types[typeName] = options; + } + + findCompareRanges(typeName) { + return this.types[typeName]?.compareRanges; + } +} diff --git a/entry_types/scrolled/package/src/review/api/index.js b/entry_types/scrolled/package/src/review/api/index.js new file mode 100644 index 0000000000..8ec21f2c2c --- /dev/null +++ b/entry_types/scrolled/package/src/review/api/index.js @@ -0,0 +1,5 @@ +import {ContentElementTypeRegistry} from './ContentElementTypeRegistry'; + +export const review = { + contentElementTypes: new ContentElementTypeRegistry() +}; diff --git a/entry_types/scrolled/package/src/review/index.js b/entry_types/scrolled/package/src/review/index.js index 21fe72da8f..abe5b7ca22 100644 --- a/entry_types/scrolled/package/src/review/index.js +++ b/entry_types/scrolled/package/src/review/index.js @@ -1,3 +1,4 @@ +export {review} from './api'; export {ReviewStateProvider, useCommentThreads, useCommentThread} from './ReviewStateProvider'; export {useLocatedCommentThreads} from './useLocatedCommentThreads'; export {ReviewMessageHandler} from './ReviewMessageHandler'; @@ -5,6 +6,7 @@ export {ThreadsBadge} from './ThreadsBadge'; export {Badge} from './Badge'; export {ThreadList} from './ThreadList'; export {Thread} from './Thread'; +export {ScrollHighlightedThreadIntoViewProvider} from './scrollHighlightedThreadIntoView'; export {NewThreadForm} from './NewThreadForm'; export {postCreateCommentThreadMessage, postUpdateThreadMessage} from './postMessage'; export {useCommentHighlights, decorateCommentHighlights} from './commentHighlights'; diff --git a/entry_types/scrolled/package/src/review/scrollHighlightedThreadIntoView.js b/entry_types/scrolled/package/src/review/scrollHighlightedThreadIntoView.js new file mode 100644 index 0000000000..5c045f1146 --- /dev/null +++ b/entry_types/scrolled/package/src/review/scrollHighlightedThreadIntoView.js @@ -0,0 +1,20 @@ +import React, {createContext, useContext} from 'react'; + +const ScrollHighlightedThreadIntoViewContext = createContext(false); + +// Editor sidebar panels (see ReviewView) render their thread lists in a +// scrollable container and want the highlighted thread scrolled into +// view. The preview popover positions itself near its badge and manages +// its own scrolling, so it leaves this off (the default) to avoid +// fighting that. +export function ScrollHighlightedThreadIntoViewProvider({children}) { + return ( + + {children} + + ); +} + +export function useScrollHighlightedThreadIntoView() { + return useContext(ScrollHighlightedThreadIntoViewContext); +} diff --git a/entry_types/scrolled/package/src/review/sortByRange.js b/entry_types/scrolled/package/src/review/sortByRange.js new file mode 100644 index 0000000000..aee36decf0 --- /dev/null +++ b/entry_types/scrolled/package/src/review/sortByRange.js @@ -0,0 +1,4 @@ +export function sortByRange(threads, compareRanges) { + if (!compareRanges) return threads; + return [...threads].sort((a, b) => compareRanges(a.subjectRange, b.subjectRange)); +} diff --git a/entry_types/scrolled/package/src/review/useLocatedCommentThreads.js b/entry_types/scrolled/package/src/review/useLocatedCommentThreads.js index a72db984de..d632efdeee 100644 --- a/entry_types/scrolled/package/src/review/useLocatedCommentThreads.js +++ b/entry_types/scrolled/package/src/review/useLocatedCommentThreads.js @@ -2,7 +2,9 @@ import {useMemo} from 'react'; import {useEntryStructureWithContentElements} from 'pageflow-scrolled/entryState'; +import {review} from './api'; import {useCommentThreads} from './ReviewStateProvider'; +import {sortByRange} from './sortByRange'; /** * Joins the comment threads from the review state with the entry @@ -25,8 +27,11 @@ export function useLocatedCommentThreads() { const locatedThreads = new Set(); const threads = []; - const take = (subjectType, subjectId) => { - const subjectThreads = threadsBySubject[subjectKey(subjectType, subjectId)] || []; + const take = (subjectType, subjectId, compareRanges) => { + const subjectThreads = sortByRange( + threadsBySubject[subjectKey(subjectType, subjectId)] || [], + compareRanges + ); subjectThreads.forEach(thread => locatedThreads.add(thread)); threads.push(...subjectThreads); return subjectThreads; @@ -38,7 +43,11 @@ export function useLocatedCommentThreads() { threads: take('Section', section.permaId), contentElements: section.contentElements.map(contentElement => ({ ...contentElement, - threads: take('ContentElement', contentElement.permaId) + threads: take( + 'ContentElement', + contentElement.permaId, + review.contentElementTypes.findCompareRanges(contentElement.type) + ) })) }));