Skip to content

Commit

Permalink
Refactoring + removed unused API args
Browse files Browse the repository at this point in the history
  • Loading branch information
rsimon committed Nov 21, 2024
1 parent c87ed10 commit 945c09d
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 37 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { ReactNode, useEffect, useMemo, useRef, useState } from 'react';
import { useAnnotator, useSelection } from '@annotorious/react';
import {
NOT_ANNOTATABLE_CLASS,
denormalizeRectWithOffset,
toDomRectList,
type TextAnnotation,
type TextAnnotator,
Expand Down Expand Up @@ -50,6 +49,12 @@ export interface TextAnnotationPopupContentProps {

}

const toViewportBounds = (annotationBounds: DOMRect, container: HTMLElement): DOMRect => {

This comment has been minimized.

Copy link
@oleksandr-danylchenko

oleksandr-danylchenko Nov 21, 2024

Contributor

That's a breaking change for the consumers ⚠
We relied on the exported denormalizeRectWithOffset in our custom popup component. I don't think that we should co-locate it with the popup.

const { left, top, right, bottom } = annotationBounds;
const containerBounds = container.getBoundingClientRect();
return new DOMRect(left + containerBounds.left, top + containerBounds.top, right - left, bottom - top);
}

export const TextAnnotationPopup = (props: TextAnnotationPopupProps) => {

const r = useAnnotator<TextAnnotator>();
Expand Down Expand Up @@ -102,17 +107,16 @@ export const TextAnnotationPopup = (props: TextAnnotationPopupProps) => {
if (isOpen && annotation?.id) {
refs.setPositionReference({
getBoundingClientRect: () => {
// Annotation bounds are relative to the document element
const bounds = r.state.store.getAnnotationBounds(annotation.id);
return bounds
? denormalizeRectWithOffset(bounds, r.element.getBoundingClientRect())
? toViewportBounds(bounds, r.element)
: new DOMRect();
},
getClientRects: () => {
const rects = r.state.store.getAnnotationRects(annotation.id);
const denormalizedRects = rects.map((rect) =>
denormalizeRectWithOffset(rect, r.element.getBoundingClientRect())
);
return toDomRectList(denormalizedRects);
const viewportRects = rects.map(rect => toViewportBounds(rect, r.element));
return toDomRectList(viewportRects);
}
});
} else {
Expand Down
2 changes: 1 addition & 1 deletion packages/text-annotator/src/state/TextAnnotationStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export interface TextAnnotationStore<T extends TextAnnotation = TextAnnotation>

getAnnotationRects(id: string): DOMRect[];

getAnnotationBounds(id: string, hintX?: number, hintY?: number, buffer?: number): DOMRect | undefined;
getAnnotationBounds(id: string): DOMRect | undefined;

getAnnotationRects(id: string): DOMRect[];

Expand Down
11 changes: 1 addition & 10 deletions packages/text-annotator/src/state/TextAnnotatorState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,18 +114,9 @@ export const createTextAnnotatorState = <I extends TextAnnotation = TextAnnotati
return all ? filtered : filtered[0];
}

const getAnnotationBounds = (id: string, x?: number, y?: number, buffer = 5): DOMRect | undefined => {
const getAnnotationBounds = (id: string): DOMRect | undefined => {
const rects = tree.getAnnotationRects(id);
if (rects.length === 0) return;

if (x && y) {
const match = rects.find(({ top, right, bottom, left }) =>
x >= left - buffer && x <= right + buffer && y >= top - buffer && y <= bottom + buffer);

// Preferred bounds: the rectangle
if (match) return match;
}

return tree.getAnnotationBounds(id);
}

Expand Down
17 changes: 6 additions & 11 deletions packages/text-annotator/src/state/spatialTree.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import RBush from 'rbush';
import type { Store } from '@annotorious/core';
import type { TextAnnotation, TextAnnotationTarget } from '../model';
import {
isRevived,
reviveSelector,
mergeClientRects,
normalizeRectWithOffset
} from '../utils';
import { isRevived, reviveSelector, mergeClientRects } from '../utils';
import type { AnnotationRects } from './TextAnnotationStore';

interface IndexedHighlightRect {
Expand Down Expand Up @@ -42,11 +37,11 @@ export const createSpatialTree = <T extends TextAnnotation>(store: Store<T>, con
return Array.from(revivedRange.getClientRects());
});

/**
* Offset the merged client rects so that coords
* are relative to the parent container
*/
const merged = mergeClientRects(rects).map(rect => normalizeRectWithOffset(rect, offset));
const merged = mergeClientRects(rects)
// Offset the merged client rects so that coords
// are relative to the parent container
.map(({ left, top, right, bottom }) =>
new DOMRect(left - offset.left, top - offset.top, right - left, bottom - top));

This comment has been minimized.

Copy link
@oleksandr-danylchenko

oleksandr-danylchenko Nov 21, 2024

Contributor

Why not make the "normalization" process more explicit my extracting it into a separate method? I would vote to rollback this change.


return merged.map(rect => {
const { x, y, width, height } = rect;
Expand Down
9 changes: 0 additions & 9 deletions packages/text-annotator/src/utils/normalizeRects.ts

This file was deleted.

2 comments on commit 945c09d

@oleksandr-danylchenko
Copy link
Contributor

Choose a reason for hiding this comment

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

See:

Moving the discussion here for better traceability

@oleksandr-danylchenko
Copy link
Contributor

Choose a reason for hiding this comment

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

UPD: Reverted this commit in #182

Please sign in to comment.