Skip to content

Commit dc9deea

Browse files
Conrad Chanmergify[bot]
Conrad Chan
and
mergify[bot]
authored
refactor(regions): Split regions to separate manager (#617)
* refactor(regions): Split regions to separate manager * chore: add unit tests * chore: address pr comments Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
1 parent 17d25f2 commit dc9deea

18 files changed

+513
-366
lines changed

‎src/document/DocumentAnnotator.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import BaseAnnotator, { ANNOTATION_CLASSES, Options } from '../common/BaseAnnota
22
import BaseManager from '../common/BaseManager';
33
import PopupManager from '../popup/PopupManager';
44
import { centerHighlight, isHighlight } from '../highlight/highlightUtil';
5-
import { centerRegion, isRegion, RegionManager } from '../region';
5+
import { centerRegion, isRegion, RegionCreationManager, RegionManager } from '../region';
66
import { Event } from '../@types';
77
import { getAnnotation } from '../store/annotations';
88
import { getSelection } from './docUtil';
@@ -83,11 +83,13 @@ export default class DocumentAnnotator extends BaseAnnotator {
8383
managers.add(new HighlightManager({ location: pageNumber, referenceEl: pageReferenceEl }));
8484
}
8585

86+
managers.add(new RegionManager({ location: pageNumber, referenceEl: pageReferenceEl }));
87+
8688
const canvasLayerEl = pageEl.querySelector<HTMLElement>('.canvasWrapper');
8789
const referenceEl =
8890
this.isFeatureEnabled('discoverability') && canvasLayerEl ? canvasLayerEl : pageReferenceEl;
8991

90-
managers.add(new RegionManager({ location: pageNumber, referenceEl }));
92+
managers.add(new RegionCreationManager({ location: pageNumber, referenceEl }));
9193
}
9294

9395
return managers;

‎src/document/__tests__/DocumentAnnotator-test.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import BaseManager from '../../common/BaseManager';
22
import DocumentAnnotator from '../DocumentAnnotator';
33
import HighlightListener from '../../highlight/HighlightListener';
44
import PopupManager from '../../popup/PopupManager';
5+
import RegionCreationManager from '../../region/RegionCreationManager';
56
import RegionManager from '../../region/RegionManager';
67
import { Annotation, Event } from '../../@types';
78
import { ANNOTATION_CLASSES } from '../../common/BaseAnnotator';
@@ -13,6 +14,7 @@ import { scrollToLocation } from '../../utils/scroll';
1314

1415
jest.mock('../../highlight/HighlightManager');
1516
jest.mock('../../popup/PopupManager');
17+
jest.mock('../../region/RegionCreationManager');
1618
jest.mock('../../region/RegionManager');
1719
jest.mock('../../utils/scroll');
1820

@@ -137,6 +139,7 @@ describe('DocumentAnnotator', () => {
137139

138140
expect(managerIterator.next().value).toBeInstanceOf(PopupManager);
139141
expect(managerIterator.next().value).toBeInstanceOf(RegionManager);
142+
expect(managerIterator.next().value).toBeInstanceOf(RegionCreationManager);
140143
});
141144

142145
test('should create HighlightManager if feature is enabled', () => {
@@ -148,6 +151,7 @@ describe('DocumentAnnotator', () => {
148151
expect(managerIterator.next().value).toBeInstanceOf(PopupManager);
149152
expect(managerIterator.next().value).toBeInstanceOf(HighlightManager);
150153
expect(managerIterator.next().value).toBeInstanceOf(RegionManager);
154+
expect(managerIterator.next().value).toBeInstanceOf(RegionCreationManager);
151155
});
152156

153157
test('should create HighlightCreatorManager if feature is enabled and textLayer is present', () => {
@@ -160,10 +164,14 @@ describe('DocumentAnnotator', () => {
160164
expect(managerIterator.next().value).toBeInstanceOf(HighlightCreatorManager);
161165
expect(managerIterator.next().value).toBeInstanceOf(HighlightManager);
162166
expect(managerIterator.next().value).toBeInstanceOf(RegionManager);
167+
expect(managerIterator.next().value).toBeInstanceOf(RegionCreationManager);
163168
});
164169

165170
test('should destroy any existing managers if they are not present in a given page element', () => {
166-
const mockManager = ({ destroy: jest.fn(), exists: jest.fn(() => false) } as unknown) as RegionManager;
171+
const mockManager = ({
172+
destroy: jest.fn(),
173+
exists: jest.fn(() => false),
174+
} as unknown) as RegionCreationManager;
167175

168176
annotator.managers.set(1, new Set([mockManager]));
169177
annotator.getPageManagers(getPage());

‎src/image/ImageAnnotator.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import BaseAnnotator, { Options } from '../common/BaseAnnotator';
33
import BaseManager from '../common/BaseManager';
44
import PopupManager from '../popup/PopupManager';
55
import { getAnnotation, getRotation } from '../store';
6-
import { centerRegion, getTransformedShape, isRegion, RegionManager } from '../region';
6+
import { centerRegion, getTransformedShape, isRegion, RegionCreationManager, RegionManager } from '../region';
77
import { CreatorStatus, getCreatorStatus } from '../store/creator';
88
import { scrollToLocation } from '../utils/scroll';
99
import './ImageAnnotator.scss';
@@ -50,6 +50,7 @@ export default class ImageAnnotator extends BaseAnnotator {
5050
if (this.managers.size === 0) {
5151
this.managers.add(new PopupManager({ referenceEl }));
5252
this.managers.add(new RegionManager({ referenceEl }));
53+
this.managers.add(new RegionCreationManager({ referenceEl }));
5354
}
5455

5556
return this.managers;

‎src/image/__tests__/ImageAnnotator-test.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,14 @@
11
import ImageAnnotator, { CSS_IS_DRAWING_CLASS } from '../ImageAnnotator';
22
import PopupManager from '../../popup/PopupManager';
3+
import RegionCreationManager from '../../region/RegionCreationManager';
34
import RegionManager from '../../region/RegionManager';
45
import { Annotation } from '../../@types';
56
import { CreatorStatus, fetchAnnotationsAction, setStatusAction } from '../../store';
67
import { annotations as regions } from '../../region/__mocks__/data';
78
import { scrollToLocation } from '../../utils/scroll';
89

910
jest.mock('../../popup/PopupManager');
11+
jest.mock('../../region/RegionCreationManager');
1012
jest.mock('../../region/RegionManager');
1113
jest.mock('../../utils/scroll');
1214

@@ -93,6 +95,7 @@ describe('ImageAnnotator', () => {
9395

9496
expect(managerIterator.next().value).toBeInstanceOf(PopupManager);
9597
expect(managerIterator.next().value).toBeInstanceOf(RegionManager);
98+
expect(managerIterator.next().value).toBeInstanceOf(RegionCreationManager);
9699
});
97100

98101
test('should destroy any existing managers if they are not present in the annotated element', () => {

‎src/region/RegionAnnotations.scss‎

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,9 @@
1-
.ba-RegionAnnotations-creator,
2-
.ba-RegionAnnotations-list,
3-
.ba-RegionAnnotations-target {
1+
.ba-RegionAnnotations-list {
42
position: absolute;
53
top: 0;
64
left: 0;
75
width: 100%;
86
height: 100%;
9-
}
10-
11-
.ba-RegionAnnotations-creator {
12-
pointer-events: auto;
13-
touch-action: none;
14-
}
15-
16-
.ba-RegionAnnotations-list {
177
pointer-events: none;
188

199
&.is-listening {
@@ -22,7 +12,3 @@
2212
}
2313
}
2414
}
25-
26-
.ba-RegionAnnotations-target {
27-
pointer-events: none;
28-
}

‎src/region/RegionAnnotations.tsx‎

Lines changed: 14 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -1,137 +1,29 @@
11
import * as React from 'react';
2-
import classNames from 'classnames';
3-
import RegionCreator from './RegionCreator';
42
import RegionList from './RegionList';
5-
import RegionRect, { RegionRectRef } from './RegionRect';
6-
import { AnnotationRegion, Rect } from '../@types';
7-
import { CreatorItemRegion, CreatorStatus } from '../store/creator';
3+
import { AnnotationRegion } from '../@types';
84
import './RegionAnnotations.scss';
95

106
type Props = {
117
activeAnnotationId: string | null;
128
annotations: AnnotationRegion[];
13-
isCreating: boolean;
14-
isDiscoverabilityEnabled: boolean;
15-
isRotated: boolean;
16-
location: number;
17-
resetCreator: () => void;
189
setActiveAnnotationId: (annotationId: string | null) => void;
19-
setReferenceShape: (rect: DOMRect) => void;
20-
setStaged: (staged: CreatorItemRegion | null) => void;
21-
setStatus: (status: CreatorStatus) => void;
22-
staged?: CreatorItemRegion | null;
2310
};
2411

25-
type State = {
26-
rectRef?: RegionRectRef;
27-
};
28-
29-
export default class RegionAnnotations extends React.PureComponent<Props, State> {
30-
static defaultProps = {
31-
annotations: [],
32-
isCreating: false,
33-
isDiscoverabilityEnabled: false,
34-
isRotated: false,
35-
};
36-
37-
state: State = {};
38-
39-
componentDidUpdate(_prevProps: Props, prevState: State): void {
40-
const { setReferenceShape } = this.props;
41-
const { rectRef } = this.state;
42-
const { rectRef: prevRectRef } = prevState;
43-
44-
if (prevRectRef !== rectRef && rectRef) {
45-
setReferenceShape(rectRef.getBoundingClientRect());
46-
}
47-
}
48-
49-
handleAbort = (): void => {
50-
const { resetCreator } = this.props;
51-
resetCreator();
52-
};
53-
54-
handleAnnotationActive = (annotationId: string | null): void => {
55-
const { setActiveAnnotationId } = this.props;
12+
const RegionAnnotations = (props: Props): JSX.Element => {
13+
const { activeAnnotationId, annotations, setActiveAnnotationId } = props;
5614

15+
const handleAnnotationActive = (annotationId: string | null): void => {
5716
setActiveAnnotationId(annotationId);
5817
};
5918

60-
handleStart = (): void => {
61-
const { setStaged, setStatus } = this.props;
62-
setStaged(null);
63-
setStatus(CreatorStatus.started);
64-
};
65-
66-
handleStop = (shape: Rect): void => {
67-
const { location, setStaged, setStatus } = this.props;
68-
setStaged({ location, shape });
69-
setStatus(CreatorStatus.staged);
70-
};
71-
72-
renderCreator = (): JSX.Element => {
73-
const { isCreating, isDiscoverabilityEnabled, isRotated } = this.props;
74-
const canCreate = isCreating && !isRotated;
75-
76-
return (
77-
<>
78-
{canCreate && (
79-
<RegionCreator
80-
className={classNames('ba-RegionAnnotations-creator', {
81-
'is-discoverability-enabled': isDiscoverabilityEnabled,
82-
})}
83-
onAbort={this.handleAbort}
84-
onStart={this.handleStart}
85-
onStop={this.handleStop}
86-
/>
87-
)}
88-
</>
89-
);
90-
};
91-
92-
renderList = (): JSX.Element => {
93-
const { activeAnnotationId, annotations } = this.props;
94-
95-
return (
96-
<RegionList
97-
activeId={activeAnnotationId}
98-
annotations={annotations}
99-
className="ba-RegionAnnotations-list"
100-
onSelect={this.handleAnnotationActive}
101-
/>
102-
);
103-
};
104-
105-
setRectRef = (rectRef: RegionRectRef): void => {
106-
this.setState({ rectRef });
107-
};
108-
109-
render(): JSX.Element {
110-
const { isCreating, isDiscoverabilityEnabled, isRotated, staged } = this.props;
111-
const canCreate = isCreating && !isRotated;
112-
113-
return (
114-
<>
115-
{isDiscoverabilityEnabled ? (
116-
<>
117-
{/* With discoverability, put the RegionCreator below the RegionList so that existing regions can be clicked */}
118-
{this.renderCreator()}
119-
{this.renderList()}
120-
</>
121-
) : (
122-
<>
123-
{this.renderList()}
124-
{this.renderCreator()}
125-
</>
126-
)}
19+
return (
20+
<RegionList
21+
activeId={activeAnnotationId}
22+
annotations={annotations}
23+
className="ba-RegionAnnotations-list"
24+
onSelect={handleAnnotationActive}
25+
/>
26+
);
27+
};
12728

128-
{/* Layer 3a: Staged (unsaved) annotation target, if any */}
129-
{canCreate && staged && (
130-
<div className="ba-RegionAnnotations-target">
131-
<RegionRect ref={this.setRectRef} isActive shape={staged.shape} />
132-
</div>
133-
)}
134-
</>
135-
);
136-
}
137-
}
29+
export default RegionAnnotations;

‎src/region/RegionAnnotationsContainer.tsx‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { connect } from 'react-redux';
2+
import RegionAnnotations from './RegionAnnotations';
3+
import withProviders from '../common/withProviders';
4+
import { AnnotationRegion } from '../@types';
5+
import { AppState, getActiveAnnotationId, getAnnotationsForLocation, setActiveAnnotationIdAction } from '../store';
6+
import { isRegion } from './regionUtil';
7+
8+
export type Props = {
9+
activeAnnotationId: string | null;
10+
annotations: AnnotationRegion[];
11+
};
12+
13+
export const mapStateToProps = (state: AppState, { location }: { location: number }): Props => {
14+
return {
15+
activeAnnotationId: getActiveAnnotationId(state),
16+
annotations: getAnnotationsForLocation(state, location).filter(isRegion),
17+
};
18+
};
19+
20+
export const mapDispatchToProps = {
21+
setActiveAnnotationId: setActiveAnnotationIdAction,
22+
};
23+
24+
export default connect(mapStateToProps, mapDispatchToProps)(withProviders(RegionAnnotations));

‎src/region/RegionCreation.scss‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
.ba-RegionCreation-creator,
2+
.ba-RegionCreation-target {
3+
position: absolute;
4+
top: 0;
5+
left: 0;
6+
width: 100%;
7+
height: 100%;
8+
}
9+
10+
.ba-RegionCreation-creator {
11+
pointer-events: auto;
12+
touch-action: none;
13+
}
14+
15+
.ba-RegionCreation-target {
16+
pointer-events: none;
17+
}

0 commit comments

Comments
 (0)