feat(highlight): Improve selection logic#559
Conversation
src/document/DocumentAnnotator.ts
Outdated
| // Clear previous selection | ||
| this.store.dispatch(setSelectionAction(null)); | ||
|
|
||
| if (!this.mouseEventHandlerAdded && this.annotatedEl) { |
There was a problem hiding this comment.
Would an overridden init method in DocumentAnnotator be a better place to add this?
There was a problem hiding this comment.
I think we should try to avoid overriding init. It calls render, so it doesn't seem to change much in this circumstance.
src/document/DocumentAnnotator.ts
Outdated
| export default class DocumentAnnotator extends BaseAnnotator { | ||
| annotatedEl?: HTMLElement; | ||
|
|
||
| isMouseSelecting = false; |
There was a problem hiding this comment.
Ideally, all state should live in the redux store. What is the reasoning for keeping this as an instance value?
There was a problem hiding this comment.
The store instance and annotator instance create and destroy together, so I think an instance variable in annotator is equivalent to a state in store and the variable is only used in this class.
src/document/DocumentAnnotator.ts
Outdated
| this.store.dispatch(setSelectionAction(null)); | ||
|
|
||
| if (!this.mouseEventHandlerAdded && this.annotatedEl) { | ||
| this.annotatedEl.addEventListener('mousedown', this.handleMouseDown); |
There was a problem hiding this comment.
Do these handlers work as expected for touch-enabled devices?
There was a problem hiding this comment.
I tested on Chrome developer tool and iPad simulator on Mac and they both work fine. I think most of the browsers today fire both touch events and mouse events.
src/document/DocumentAnnotator.ts
Outdated
|
|
||
| if (!this.mouseEventHandlerAdded && this.annotatedEl) { | ||
| this.annotatedEl.addEventListener('mousedown', this.handleMouseDown); | ||
| this.annotatedEl.addEventListener('mouseup', this.handleMouseUp); |
There was a problem hiding this comment.
What happens if the mouse button is released outside of the annotated element or vice versa? Do things work as desired?
There was a problem hiding this comment.
If the mouse is released outside of the annotated element or vice versa, the popup will not show, which is the desired behavior confirmed with designer.
mingzexiao6
force-pushed
the
selection-change
branch
3 times, most recently
from
0c55bb8 to
d7cb9c7
Compare
|
|
||
| store: AppStore; | ||
|
|
||
| constructor({ getSelection, selectionChangeDelay = 0, store }: Options) { |
There was a problem hiding this comment.
Would it make sense to default selectionChangeDelay to the 300ms magic number?
There was a problem hiding this comment.
I think 300ms magic number is only related to pdfjs, which is only related to DocumentAnnotator. For the other annotators which don't use pdfjs, it should be 0.
src/highlight/HighlightListener.ts
Outdated
| import { MOUSE_PRIMARY } from '../constants'; | ||
|
|
||
| export type Options = { | ||
| getSelection: () => SelectionArg | null; |
There was a problem hiding this comment.
nit: feels a bit funny that the return type has name Arg in it, could Selection type be used here?
There was a problem hiding this comment.
The Selection type is defined in docUtil, which I want to separate from highlight. I can rename SelectionArg to Selection in the import.
mingzexiao6
force-pushed
the
selection-change
branch
from
ce84a2e to
3e511ab
Compare
3 participants
This PR addresses following feedbacks from product:
Todo: