Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(features): Features as an option #555

Merged
merged 3 commits into from
Aug 19, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/common/BaseAnnotator.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import getProp from 'lodash/get';
import { IntlShape } from 'react-intl';
import * as store from '../store';
import API from '../api';
Expand All @@ -9,6 +10,10 @@ import './BaseAnnotator.scss';

export type Container = string | HTMLElement;

export type Features = {
[key: string]: boolean;
};

export type FileOptions = {
[key: string]: {
annotations?: {
Expand All @@ -22,6 +27,7 @@ export type FileOptions = {
export type Options = {
apiHost: string;
container: Container;
features?: Features;
file: {
id: string;
file_version: {
Expand All @@ -46,11 +52,13 @@ export default class BaseAnnotator extends EventEmitter {

container: Container;

features: Features;

intl: IntlShape;

store: store.AppStore;

constructor({ apiHost, container, file, fileOptions, intl, token }: Options) {
constructor({ apiHost, container, features = {}, file, fileOptions, intl, token }: Options) {
ConradJChan marked this conversation as resolved.
Show resolved Hide resolved
super();

const fileOptionsValue = fileOptions?.[file.id];
Expand All @@ -71,6 +79,7 @@ export default class BaseAnnotator extends EventEmitter {
};

this.container = container;
this.features = features;
this.intl = i18n.createIntlProvider(intl);
this.store = store.createStore(initialState, {
api: new API({ apiHost, token }),
Expand Down Expand Up @@ -123,6 +132,10 @@ export default class BaseAnnotator extends EventEmitter {
this.store.dispatch(store.setIsInitialized());
}

public isFeatureEnabled(feature = ''): boolean {
return getProp(this.features, feature, false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does something like return this.features?.[feature] || false work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, but I figure getProp could be more flexible for nesting of features?

}

public removeAnnotation = (annotationId: string): void => {
this.store.dispatch(store.removeAnnotationAction(annotationId));
};
Expand Down
15 changes: 15 additions & 0 deletions src/common/__tests__/BaseAnnotator-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ describe('BaseAnnotator', () => {
const container = document.createElement('div');
container.innerHTML = `<div class="inner" />`;

const features = { enabledFeature: true };
const defaults = {
apiHost: 'https://api.box.com',
container,
features,
file: {
id: '12345',
file_version: { id: '98765' },
Expand Down Expand Up @@ -135,6 +137,12 @@ describe('BaseAnnotator', () => {
);
},
);

test('should set features option', () => {
annotator = getAnnotator({ features });

expect(annotator.features).toEqual(features);
});
});

describe('destroy()', () => {
Expand Down Expand Up @@ -251,4 +259,11 @@ describe('BaseAnnotator', () => {
expect(annotator.store.dispatch).toBeCalledWith(store.toggleAnnotationModeAction('region' as Mode));
});
});

describe('isFeatureEnabled()', () => {
test('should return whether feature is enabled or not', () => {
expect(annotator.isFeatureEnabled('enabledFeature')).toBeTruthy();
expect(annotator.isFeatureEnabled('notEnabledFeature')).toBeFalsy();
ConradJChan marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
4 changes: 4 additions & 0 deletions src/document/DocumentAnnotator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { centerRegion, isRegion, RegionManager } from '../region';
import { Event } from '../@types';
import { getAnnotation } from '../store/annotations';
import { getSelection } from './docUtil';
import { HighlightManager } from '../highlight';
import { Mode, setSelectionAction } from '../store';
import { scrollToLocation } from '../utils/scroll';
import './DocumentAnnotator.scss';
Expand Down Expand Up @@ -56,6 +57,9 @@ export default class DocumentAnnotator extends BaseAnnotator {

// Lazily instantiate managers as pages are added or re-rendered
if (managers.size === 0) {
if (this.isFeatureEnabled('highlightText')) {
managers.add(new HighlightManager({ location: pageNumber, referenceEl: pageReferenceEl }));
}
managers.add(new RegionManager({ location: pageNumber, referenceEl: pageReferenceEl }));
}

Expand Down
12 changes: 12 additions & 0 deletions src/document/__tests__/DocumentAnnotator-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import RegionManager from '../../region/RegionManager';
import { Annotation, Event } from '../../@types';
import { annotations as regions } from '../../region/__mocks__/data';
import { fetchAnnotationsAction, setSelectionAction } from '../../store';
import { HighlightManager } from '../../highlight';
import { mockRange } from '../../store/selection/__mocks__/range';
import { scrollToLocation } from '../../utils/scroll';

Expand Down Expand Up @@ -129,6 +130,17 @@ describe('DocumentAnnotator', () => {
expect(managerIterator.next().value).toBeInstanceOf(RegionManager);
});

test('should create HighlightManager if feature is enabled', () => {
annotator = getAnnotator({ features: { highlightText: true } });

const managers = annotator.getPageManagers(getPage());
const managerIterator = managers.values();

expect(managers.size).toBe(2);
expect(managerIterator.next().value).toBeInstanceOf(HighlightManager);
expect(managerIterator.next().value).toBeInstanceOf(RegionManager);
});

test('should destroy any existing managers if they are not present in a given page element', () => {
const mockManager = ({ destroy: jest.fn(), exists: jest.fn(() => false) } as unknown) as RegionManager;

Expand Down