feat(highlight): Promote and show staged highlight annotation #563
Conversation
mxiao6
commented
Aug 27, 2020
•
mxiao6
commented
- Tests
| // empty list, push current | ||
| if (!prev) { | ||
| dedupedRects.push(curr); | ||
| return; |
There was a problem hiding this comment.
We generally try to avoid multiple returns (outside of guard clauses) because it can make the function more difficult to step through.
There was a problem hiding this comment.
The pop() may return undefined, so this is a guard clause. I removed an unnecessary return below.
mxiao6
force-pushed
the
promote-staged-2
branch
from
603dc0b to
d8741a2
Compare
src/store/promoter/actions.ts
Outdated
| import { SelectionItem } from './types'; | ||
| import { Shape } from '../../@types'; | ||
|
|
||
| export type SelectionArg = { | ||
| location: number; | ||
| pageRect: DOMRect; |
There was a problem hiding this comment.
Does this need to be stored?
There was a problem hiding this comment.
Yeah, it's needed. If we get page rect on rendering, and if user selects text then scrolls page before clicks promotion button, the page x/y will not match with selection x/y. Have to get them and store them at the same moment.
mxiao6
force-pushed
the
promote-staged-2
branch
from
d8741a2 to
6197ae0
Compare
| groups[roundedY] = groups[roundedY] || []; | ||
| groups[roundedY].push(rect); | ||
| return groups; | ||
| }, {} as Record<number, Shape[]>); |
There was a problem hiding this comment.
Would we be able to get rid of this cast if we defined rows as Record<number, Shape[]>?
There was a problem hiding this comment.
Tried, doesn't work. It seems like TS is not able to infer accumulator's type from result's type, unfortunately.
Renaming will change a lot of files, which makes harder to review this PR. I'm thinking to do it in a separate PR. |
mxiao6
force-pushed
the
promote-staged-2
branch
2 times, most recently
from
f43f7ff to
36c7ec9
Compare
| const xThreshold = getThreshold(prevWidth, width, threshold); | ||
| const yThreshold = getThreshold(prevHeight, height, threshold); | ||
|
|
||
| if (Math.abs(prevX - x) <= xThreshold && Math.abs(prevY - y) <= yThreshold) { |
There was a problem hiding this comment.
Should width and height still be taken into account in determining duplication?
There was a problem hiding this comment.
If two rects have the same x and y, one must be the duplicate of the other, right? So I think only comparing x and y is enough for determining duplications.
mxiao6
force-pushed
the
promote-staged-2
branch
2 times, most recently
from
584391b to
1766326
Compare
mxiao6
force-pushed
the
promote-staged-2
branch
from
1766326 to
44c8e45
Compare
