-
Notifications
You must be signed in to change notification settings - Fork 47
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
refactor: Move deselect and isListening logic #662
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous logic was always a little questionable, so this seems like a good change overall. I'm wary of global event handling sprawl, especially in the base annotator, since that was a contributor to some of the issues in prior versions of the library. It should be fine here, but should be cautious about adding more of these.
One other question is whether this works correctly with multiple preview instances within the same document. Does that cause any weird behavior?
src/common/BaseAnnotator.ts
Outdated
@@ -141,6 +143,8 @@ export default class BaseAnnotator extends EventEmitter { | |||
this.annotatedEl.classList.add(CSS_LOADED_CLASS); | |||
this.containerEl.classList.add(CSS_CONTAINER_CLASS); | |||
|
|||
document.addEventListener('mousedown', this.handleDeselect); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init
method can be called many times. Should we have a guard against this being added more than once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I could add a guard clause here or would adding it in the constructor make more sense?
|
||
annotator.destroy(); | ||
|
||
expect(removeEventListenerSpy).toHaveBeenCalledWith('mousedown', expect.any(Function)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to pass annotator.handleDeselect
as the second parameter here. Same for the other new test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since its visibility is protected
typescript complains about attempting to reference it. It appears the other event callbacks follow this expect.any(Function)
approach as well. I tried looking for a disable rule for the tslint but haven't been able to find one
I think having multiple preview instances on the same document should be ok because this document |
This PR changes the logic for how deselecting the
activeId
is handled as well as how each*List
component determines whether the existing rendered annotations are listening for interaction events.Deselect
activeId
Deselecting the
activeId
is now moved to theBaseAnnotator
which listens for themousedown
event on the document. One benefit of this is that it is done once for all annotators vs once per page per type of annotation, i.e. 1 vs 1 * 3 * 3 = 9 (assuming a 3 page document supporting region, highlight, drawing). One drawback is that it no longer benefits from theuseOutsideClick
logic that was in place to ignoremousedown
events that originated from the*List
component's root element but that should be minimal since multi page documents continued to trigger the same logic anyway whenmousedown
events occurred on other pages.isListening
isListening
is now determined based on two pieces of store state: (1)CreatorStatus
from the creator store and (2)isSelecting
from the highlight store. IfCreatorStatus
isinit
andisSelecting
isfalse
then the existing rendered annotations should be listening for interaction events. The benefit of this change is that the individual*List
components do not need to maintain their own state for this.TODO: