Commit 6c3eb16f authored by Robert Knight's avatar Robert Knight

Display page numbers on annotation cards if feature enabled

Display page numbers on annotation cards when:

 - The `page_numbers` feature flag is enabled AND
 - The annotation has a `PageSelector` selector. This applies to VitalSource
   books and PDFs.

Part of https://github.com/hypothesis/client/issues/5986
parent 9856f03d
...@@ -12,6 +12,7 @@ import { ...@@ -12,6 +12,7 @@ import {
isHighlight, isHighlight,
isReply, isReply,
hasBeenEdited, hasBeenEdited,
pageLabel as getPageLabel,
} from '../../helpers/annotation-metadata'; } from '../../helpers/annotation-metadata';
import { import {
annotationAuthorLink, annotationAuthorLink,
...@@ -54,6 +55,7 @@ function AnnotationHeader({ ...@@ -54,6 +55,7 @@ function AnnotationHeader({
const defaultAuthority = store.defaultAuthority(); const defaultAuthority = store.defaultAuthority();
const displayNamesEnabled = store.isFeatureEnabled('client_display_names'); const displayNamesEnabled = store.isFeatureEnabled('client_display_names');
const userURL = store.getLink('user', { user: annotation.user }); const userURL = store.getLink('user', { user: annotation.user });
const pageNumbersEnabled = store.isFeatureEnabled('page_numbers');
const authorName = useMemo( const authorName = useMemo(
() => () =>
...@@ -101,7 +103,14 @@ function AnnotationHeader({ ...@@ -101,7 +103,14 @@ function AnnotationHeader({
// an ID. // an ID.
store.setExpanded(annotation.id!, true); store.setExpanded(annotation.id!, true);
const group = store.getGroup(annotation.group); // As part of the `page_numbers` feature, we are hiding the group on cards in
// contexts where it is the same for all cards and is shown elsewhere in the
// UI (eg. the top bar). This is to reduce visual clutter.
let group;
if (!pageNumbersEnabled || store.route() !== 'sidebar') {
group = store.getGroup(annotation.group);
}
const pageNumber = pageNumbersEnabled ? getPageLabel(annotation) : undefined;
return ( return (
<header> <header>
...@@ -153,6 +162,8 @@ function AnnotationHeader({ ...@@ -153,6 +162,8 @@ function AnnotationHeader({
className="w-[10px] h-[10px] text-color-text-light" className="w-[10px] h-[10px] text-color-text-light"
/> />
)} )}
{(showDocumentInfo || pageNumber) && (
<span className="flex">
{showDocumentInfo && ( {showDocumentInfo && (
<AnnotationDocumentInfo <AnnotationDocumentInfo
domain={documentInfo.domain} domain={documentInfo.domain}
...@@ -160,6 +171,13 @@ function AnnotationHeader({ ...@@ -160,6 +171,13 @@ function AnnotationHeader({
title={documentInfo.titleText} title={documentInfo.titleText}
/> />
)} )}
{pageNumber && (
<span className="text-grey-6" data-testid="page-number">
{showDocumentInfo && ', '}p.{pageNumber}
</span>
)}
</span>
)}
</div> </div>
)} )}
</header> </header>
......
...@@ -8,6 +8,7 @@ import * as fixtures from '../../../test/annotation-fixtures'; ...@@ -8,6 +8,7 @@ import * as fixtures from '../../../test/annotation-fixtures';
import AnnotationHeader, { $imports } from '../AnnotationHeader'; import AnnotationHeader, { $imports } from '../AnnotationHeader';
describe('AnnotationHeader', () => { describe('AnnotationHeader', () => {
let activeFeatures;
let fakeAnnotationAuthorLink; let fakeAnnotationAuthorLink;
let fakeAnnotationDisplayName; let fakeAnnotationDisplayName;
let fakeDomainAndTitle; let fakeDomainAndTitle;
...@@ -33,6 +34,11 @@ describe('AnnotationHeader', () => { ...@@ -33,6 +34,11 @@ describe('AnnotationHeader', () => {
}; };
beforeEach(() => { beforeEach(() => {
activeFeatures = {
client_display_names: true,
page_numbers: false,
};
fakeAnnotationAuthorLink = sinon fakeAnnotationAuthorLink = sinon
.stub() .stub()
.returns('http://www.example.com/user/'); .returns('http://www.example.com/user/');
...@@ -54,10 +60,13 @@ describe('AnnotationHeader', () => { ...@@ -54,10 +60,13 @@ describe('AnnotationHeader', () => {
fakeStore = { fakeStore = {
defaultAuthority: sinon.stub().returns('example.com'), defaultAuthority: sinon.stub().returns('example.com'),
isFeatureEnabled: sinon isFeatureEnabled: sinon.stub().callsFake(feature => {
.stub() const enabled = activeFeatures[feature];
.withArgs('client_display_names') if (enabled === undefined) {
.returns(true), throw new Error(`Unknown feature "${feature}"`);
}
return enabled;
}),
getGroup: sinon.stub().returns(fakeGroup), getGroup: sinon.stub().returns(fakeGroup),
getLink: sinon getLink: sinon
.stub() .stub()
...@@ -443,6 +452,52 @@ describe('AnnotationHeader', () => { ...@@ -443,6 +452,52 @@ describe('AnnotationHeader', () => {
}); });
}); });
context('when page_numbers feature is enabled', () => {
beforeEach(() => {
activeFeatures.page_numbers = true;
// Un-mock the `pageLabel` function.
$imports.$restore({
'../../helpers/annotation-metadata': true,
});
});
it('should not display page number if missing', () => {
const annotation = fixtures.defaultAnnotation();
const wrapper = createAnnotationHeader({ annotation });
assert.isFalse(wrapper.exists('[data-testid="page-number"]'));
});
it('should display page number if available', () => {
const annotation = fixtures.defaultAnnotation();
annotation.target[0].selector.push({
type: 'PageSelector',
index: 10,
label: '11',
});
const wrapper = createAnnotationHeader({ annotation });
const pageNumber = wrapper.find('[data-testid="page-number"]');
assert.isTrue(pageNumber.exists());
assert.equal(pageNumber.text(), 'p.11');
});
it('should hide group name in sidebar', () => {
fakeStore.route.returns('sidebar');
const wrapper = createAnnotationHeader({
annotation: fixtures.defaultAnnotation(),
});
assert.isFalse(wrapper.exists('AnnotationShareInfo'));
});
it('should still show group name outside the sidebar', () => {
fakeStore.route.returns('annotation');
const wrapper = createAnnotationHeader({
annotation: fixtures.defaultAnnotation(),
});
assert.isTrue(wrapper.exists('AnnotationShareInfo'));
});
});
it( it(
'should pass a11y checks', 'should pass a11y checks',
checkAccessibility([ checkAccessibility([
......
import type { import type {
APIAnnotationData, APIAnnotationData,
Annotation, Annotation,
PageSelector,
SavedAnnotation, SavedAnnotation,
TextQuoteSelector, TextQuoteSelector,
} from '../../types/api'; } from '../../types/api';
...@@ -341,6 +342,18 @@ export function quote(annotation: APIAnnotationData): string | null { ...@@ -341,6 +342,18 @@ export function quote(annotation: APIAnnotationData): string | null {
return quoteSel ? quoteSel.exact : null; return quoteSel ? quoteSel.exact : null;
} }
/**
* Return the label of the page that an annotation comes from.
*
* This is usually a 1-based page number, but can also be roman numerals etc.
*/
export function pageLabel(annotation: APIAnnotationData): string | undefined {
const pageSel = annotation.target[0]?.selector?.find(
s => s.type === 'PageSelector',
) as PageSelector | undefined;
return pageSel?.label;
}
/** /**
* Has this annotation been edited subsequent to its creation? * Has this annotation been edited subsequent to its creation?
*/ */
......
...@@ -4,6 +4,7 @@ import { ...@@ -4,6 +4,7 @@ import {
documentMetadata, documentMetadata,
domainAndTitle, domainAndTitle,
isSaved, isSaved,
pageLabel,
} from '../annotation-metadata'; } from '../annotation-metadata';
describe('sidebar/helpers/annotation-metadata', () => { describe('sidebar/helpers/annotation-metadata', () => {
...@@ -587,6 +588,27 @@ describe('sidebar/helpers/annotation-metadata', () => { ...@@ -587,6 +588,27 @@ describe('sidebar/helpers/annotation-metadata', () => {
}); });
}); });
describe('pageLabel', () => {
it('returns page label for annotation', () => {
const ann = {
target: [
{
source: 'https://publisher.org/article.pdf',
selector: [{ type: 'PageSelector', index: 10, label: '11' }],
},
],
};
assert.equal(pageLabel(ann), '11');
});
it('returns undefined if annotation has no `PageSelector` selector', () => {
const anns = [fixtures.newPageNote(), fixtures.newAnnotation()];
for (const ann of anns) {
assert.isUndefined(pageLabel(ann));
}
});
});
describe('hasBeenEdited', () => { describe('hasBeenEdited', () => {
it('should return false if created and updated timestamps are equal', () => { it('should return false if created and updated timestamps are equal', () => {
const annotation = fakeAnnotation({ const annotation = fakeAnnotation({
......
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