Commit c7ad6c67 authored by Alejandro Celaya's avatar Alejandro Celaya Committed by Alejandro Celaya

Highlight annotations when they are applied from pending updates

parent f2e80a6d
...@@ -105,11 +105,14 @@ function Annotation({ ...@@ -105,11 +105,14 @@ function Annotation({
const annotationDescription = isSaved(annotation) const annotationDescription = isSaved(annotation)
? annotationRole(annotation) ? annotationRole(annotation)
: `New ${annotationRole(annotation).toLowerCase()}`; : `New ${annotationRole(annotation).toLowerCase()}`;
const state = store.isAnnotationHighlighted(annotation)
? ' - Highlighted'
: '';
return ( return (
<article <article
className="space-y-4" className="space-y-4"
aria-label={`${annotationDescription} by ${authorName}`} aria-label={`${annotationDescription} by ${authorName}${state}`}
> >
<AnnotationHeader <AnnotationHeader
annotation={annotation} annotation={annotation}
......
...@@ -3,6 +3,7 @@ import { ...@@ -3,6 +3,7 @@ import {
mockImportedComponents, mockImportedComponents,
} from '@hypothesis/frontend-testing'; } from '@hypothesis/frontend-testing';
import { mount } from 'enzyme'; import { mount } from 'enzyme';
import sinon from 'sinon';
import * as fixtures from '../../../test/annotation-fixtures'; import * as fixtures from '../../../test/annotation-fixtures';
import Annotation, { $imports } from '../Annotation'; import Annotation, { $imports } from '../Annotation';
...@@ -64,6 +65,7 @@ describe('Annotation', () => { ...@@ -64,6 +65,7 @@ describe('Annotation', () => {
isSavingAnnotation: sinon.stub().returns(false), isSavingAnnotation: sinon.stub().returns(false),
profile: sinon.stub().returns({ userid: 'acct:foo@bar.com' }), profile: sinon.stub().returns({ userid: 'acct:foo@bar.com' }),
setExpanded: sinon.stub(), setExpanded: sinon.stub(),
isAnnotationHighlighted: sinon.stub().returns(false),
}; };
$imports.$mock(mockImportedComponents()); $imports.$mock(mockImportedComponents());
...@@ -96,6 +98,18 @@ describe('Annotation', () => { ...@@ -96,6 +98,18 @@ describe('Annotation', () => {
'New annotation by Richard Lionheart', 'New annotation by Richard Lionheart',
); );
}); });
[true, false].forEach(isHighlighted => {
it('should mention if annotation is highlighted', () => {
fakeStore.isAnnotationHighlighted.returns(isHighlighted);
const wrapper = createComponent();
assert.equal(
wrapper.find('article').prop('aria-label').endsWith(' - Highlighted'),
isHighlighted,
);
});
});
}); });
describe('annotation quote', () => { describe('annotation quote', () => {
......
...@@ -60,8 +60,6 @@ function AnnotationView({ ...@@ -60,8 +60,6 @@ function AnnotationView({
// not shown until the user expands the thread. // not shown until the user expands the thread.
annots.forEach(annot => annot.id && store.setExpanded(annot.id, true)); annots.forEach(annot => annot.id && store.setExpanded(annot.id, true));
// FIXME - This should show a visual indication of which reply the
// annotation ID in the URL refers to. That isn't currently working.
if (topLevelAnnot.id !== annotationId) { if (topLevelAnnot.id !== annotationId) {
store.highlightAnnotations([annotationId]); store.highlightAnnotations([annotationId]);
} }
......
import { Card, CardContent } from '@hypothesis/frontend-shared'; import { Card, CardContent } from '@hypothesis/frontend-shared';
import classnames from 'classnames';
import debounce from 'lodash.debounce'; import debounce from 'lodash.debounce';
import { useCallback, useEffect, useMemo, useRef } from 'preact/hooks'; import { useCallback, useEffect, useMemo, useRef } from 'preact/hooks';
...@@ -29,6 +30,7 @@ function ThreadCard({ frameSync, thread }: ThreadCardProps) { ...@@ -29,6 +30,7 @@ function ThreadCard({ frameSync, thread }: ThreadCardProps) {
debounce((ann: Annotation | null) => frameSync.hoverAnnotation(ann), 10), debounce((ann: Annotation | null) => frameSync.hoverAnnotation(ann), 10),
[frameSync], [frameSync],
); );
const isHighlighted = store.isAnnotationHighlighted(thread.annotation);
const scrollToAnnotation = useCallback( const scrollToAnnotation = useCallback(
(ann: Annotation) => { (ann: Annotation) => {
...@@ -65,7 +67,10 @@ function ThreadCard({ frameSync, thread }: ThreadCardProps) { ...@@ -65,7 +67,10 @@ function ThreadCard({ frameSync, thread }: ThreadCardProps) {
return ( return (
<Card <Card
active={isHovered} active={isHovered}
classes="cursor-pointer focus-visible-ring theme-clean:border-none" classes={classnames(
'cursor-pointer focus-visible-ring theme-clean:border-none',
{ 'border-brand': isHighlighted },
)}
data-testid="thread-card" data-testid="thread-card"
elementRef={cardRef} elementRef={cardRef}
tabIndex={-1} tabIndex={-1}
......
...@@ -36,6 +36,7 @@ describe('ThreadCard', () => { ...@@ -36,6 +36,7 @@ describe('ThreadCard', () => {
clearAnnotationFocusRequest: sinon.stub(), clearAnnotationFocusRequest: sinon.stub(),
isAnnotationHovered: sinon.stub().returns(false), isAnnotationHovered: sinon.stub().returns(false),
route: sinon.stub(), route: sinon.stub(),
isAnnotationHighlighted: sinon.stub().returns(false),
}; };
fakeThread = { fakeThread = {
...@@ -141,6 +142,18 @@ describe('ThreadCard', () => { ...@@ -141,6 +142,18 @@ describe('ThreadCard', () => {
}); });
}); });
[true, false].forEach(isHighlighted => {
it('applies UI changes when annotation is highlighted', () => {
fakeStore.isAnnotationHighlighted.returns(isHighlighted);
const wrapper = createComponent();
assert.equal(
wrapper.find('Card').prop('classes').includes('border-brand'),
isHighlighted,
);
});
});
it( it(
'should pass a11y checks', 'should pass a11y checks',
checkAccessibility({ checkAccessibility({
......
...@@ -52,6 +52,9 @@ export class StreamerService { ...@@ -52,6 +52,9 @@ export class StreamerService {
*/ */
private _reconnectionAttempts: number; private _reconnectionAttempts: number;
// Test seam
private _window: Window;
/** The randomly generated session ID */ /** The randomly generated session ID */
clientId: string; clientId: string;
...@@ -61,6 +64,7 @@ export class StreamerService { ...@@ -61,6 +64,7 @@ export class StreamerService {
auth: AuthService, auth: AuthService,
groups: GroupsService, groups: GroupsService,
session: SessionService, session: SessionService,
$window: Window,
) { ) {
this._auth = auth; this._auth = auth;
this._groups = groups; this._groups = groups;
...@@ -75,6 +79,7 @@ export class StreamerService { ...@@ -75,6 +79,7 @@ export class StreamerService {
this._configMessages = {}; this._configMessages = {};
this._reconnectionAttempts = 0; this._reconnectionAttempts = 0;
this._reconnectSetUp = false; this._reconnectSetUp = false;
this._window = $window;
} }
/** /**
...@@ -85,6 +90,11 @@ export class StreamerService { ...@@ -85,6 +90,11 @@ export class StreamerService {
const updates = Object.values(this._store.pendingUpdates()); const updates = Object.values(this._store.pendingUpdates());
if (updates.length) { if (updates.length) {
this._store.addAnnotations(updates); this._store.addAnnotations(updates);
// Highlight the new/edited annotations for 5 seconds
this._store.highlightAnnotations(
updates.map(({ id }) => id).filter(Boolean) as string[],
);
this._window.setTimeout(() => this._store.highlightAnnotations([]), 5000);
} }
const deletions = Object.keys(this._store.pendingDeletions()).map(id => ({ const deletions = Object.keys(this._store.pendingDeletions()).map(id => ({
......
import { delay } from '@hypothesis/frontend-testing'; import { delay } from '@hypothesis/frontend-testing';
import sinon from 'sinon';
import EventEmitter from 'tiny-emitter'; import EventEmitter from 'tiny-emitter';
import { promiseWithResolvers } from '../../../shared/promise-with-resolvers';
import { fakeReduxStore } from '../../test/fake-redux-store'; import { fakeReduxStore } from '../../test/fake-redux-store';
import { StreamerService, $imports } from '../streamer'; import { StreamerService, $imports } from '../streamer';
...@@ -81,6 +83,7 @@ describe('StreamerService', () => { ...@@ -81,6 +83,7 @@ describe('StreamerService', () => {
let fakeSession; let fakeSession;
let fakeWarnOnce; let fakeWarnOnce;
let activeStreamer; let activeStreamer;
let fakeSetTimeout;
function createDefaultStreamer() { function createDefaultStreamer() {
activeStreamer = new StreamerService( activeStreamer = new StreamerService(
...@@ -89,9 +92,22 @@ describe('StreamerService', () => { ...@@ -89,9 +92,22 @@ describe('StreamerService', () => {
fakeAuth, fakeAuth,
fakeGroups, fakeGroups,
fakeSession, fakeSession,
{ setTimeout: fakeSetTimeout },
); );
} }
function timeoutAsPromise() {
const { resolve, promise } = promiseWithResolvers();
fakeSetTimeout.callsFake(callback =>
setTimeout(() => {
callback();
resolve();
}, 0),
);
return promise;
}
beforeEach(() => { beforeEach(() => {
fakeAPIRoutes = { fakeAPIRoutes = {
links: sinon.stub().resolves({ websocket: 'ws://example.com/ws' }), links: sinon.stub().resolves({ websocket: 'ws://example.com/ws' }),
...@@ -114,6 +130,7 @@ describe('StreamerService', () => { ...@@ -114,6 +130,7 @@ describe('StreamerService', () => {
}), }),
receiveRealTimeUpdates: sinon.stub(), receiveRealTimeUpdates: sinon.stub(),
removeAnnotations: sinon.stub(), removeAnnotations: sinon.stub(),
highlightAnnotations: sinon.stub(),
}, },
); );
...@@ -127,6 +144,7 @@ describe('StreamerService', () => { ...@@ -127,6 +144,7 @@ describe('StreamerService', () => {
}; };
fakeWarnOnce = sinon.stub(); fakeWarnOnce = sinon.stub();
fakeSetTimeout = sinon.stub();
$imports.$mock({ $imports.$mock({
'../../shared/warn-once': { warnOnce: fakeWarnOnce }, '../../shared/warn-once': { warnOnce: fakeWarnOnce },
...@@ -388,7 +406,8 @@ describe('StreamerService', () => { ...@@ -388,7 +406,8 @@ describe('StreamerService', () => {
return activeStreamer.connect(); return activeStreamer.connect();
}); });
it('applies updates immediately', () => { it('applies updates immediately and highlights annotations', async () => {
const timeoutPromise = timeoutAsPromise();
const [ann] = fixtures.createNotification.payload; const [ann] = fixtures.createNotification.payload;
fakeStore.pendingUpdates.returns({ fakeStore.pendingUpdates.returns({
[ann.id]: ann, [ann.id]: ann,
...@@ -403,6 +422,14 @@ describe('StreamerService', () => { ...@@ -403,6 +422,14 @@ describe('StreamerService', () => {
fakeStore.addAnnotations, fakeStore.addAnnotations,
fixtures.createNotification.payload, fixtures.createNotification.payload,
); );
assert.calledWith(
fakeStore.highlightAnnotations,
fixtures.createNotification.payload.map(({ id }) => id),
);
assert.calledWith(fakeSetTimeout, sinon.match.func, 5000);
await timeoutPromise;
assert.calledWith(fakeStore.highlightAnnotations, []);
}); });
}); });
......
...@@ -384,8 +384,9 @@ function hideAnnotation(id: string) { ...@@ -384,8 +384,9 @@ function hideAnnotation(id: string) {
/** /**
* Highlight annotations with the given `ids`. * Highlight annotations with the given `ids`.
* *
* This is used to indicate the specific annotation in a thread that was * This is used to add a visual indicator to specific annotation cards, like a
* linked to for example. Replaces the current map of highlighted annotations. * thread that was linked or annotations from pending updates that were applied.
* Replaces the current map of highlighted annotations.
* All provided annotations (`ids`) will be set to `true` in the `highlighted` * All provided annotations (`ids`) will be set to `true` in the `highlighted`
* map. * map.
*/ */
...@@ -498,6 +499,13 @@ const highlightedAnnotations = createSelector( ...@@ -498,6 +499,13 @@ const highlightedAnnotations = createSelector(
highlighted => trueKeys(highlighted), highlighted => trueKeys(highlighted),
); );
/**
* Is the annotation currently highlighted?
*/
function isAnnotationHighlighted(state: State, annotation?: Annotation) {
return !!annotation?.id && state.highlighted[annotation.id] === true;
}
/** /**
* Is the annotation identified by `$tag` currently hovered? * Is the annotation identified by `$tag` currently hovered?
*/ */
...@@ -581,6 +589,7 @@ export const annotationsModule = createStoreModule(initialState, { ...@@ -581,6 +589,7 @@ export const annotationsModule = createStoreModule(initialState, {
findIDsForTags, findIDsForTags,
hoveredAnnotations, hoveredAnnotations,
highlightedAnnotations, highlightedAnnotations,
isAnnotationHighlighted,
isAnnotationHovered, isAnnotationHovered,
isWaitingToAnchorAnnotations, isWaitingToAnchorAnnotations,
newAnnotations, newAnnotations,
......
...@@ -527,4 +527,20 @@ describe('sidebar/store/modules/annotations', () => { ...@@ -527,4 +527,20 @@ describe('sidebar/store/modules/annotations', () => {
assert.isTrue(store.annotationExists(annot.id)); assert.isTrue(store.annotationExists(annot.id));
}); });
}); });
describe('isAnnotationHighlighted', () => {
[
{ annotation: undefined, expectedResult: false },
{ annotation: {}, expectedResult: false },
{ annotation: { id: '1' }, expectedResult: true },
{ annotation: { id: '2' }, expectedResult: true },
{ annotation: { id: '3' }, expectedResult: false },
].forEach(({ annotation, expectedResult }) => {
it('returns true if the annotation ID is in the set of highlighted annotations', () => {
const store = createTestStore();
store.highlightAnnotations(['1', '2']);
assert.equal(store.isAnnotationHighlighted(annotation), expectedResult);
});
});
});
}); });
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment