#127 Made annotatingEnabled property reactive
#128
Conversation
oleksandr-danylchenko
force-pushed
the
#127-annotating-enabled-reactive
branch
from
dae32c7 to
da40d2c
Compare
| const setAnnotatingEnabled = (enabled: boolean) => { | ||
| currentAnnotatingEnabled = enabled; | ||
| onSelectionChange.clear(); | ||
|
|
||
| if (!enabled) { | ||
| currentTarget = undefined; | ||
| lastPointerDown = undefined; | ||
| } | ||
| }; |
There was a problem hiding this comment.
We must clean the onSelectionChange debounced call to prevent it from running on obsolete values from the closure. Otherwise, the selectionchange event will still get processed in the deferred call, even though the currentAnnotatingEnabled became false.
There was a problem hiding this comment.
I wonder how crucial that is though. Would annotatingEnabled get called during a selection in practice? The normal use cases I would see is:
- Setting it once on page init (because the user is getting a page served in "read-only mode")
- "Locking a page" interactively, by the user clicking a "Lock" button. (In which case there wouldn't be a selection.)
There was a problem hiding this comment.
I wonder how crucial that is, though.
It shouldn't be extremely crucial. But it's a good tone not to leave hanging callbacks/listeners that might run on obsolete values. I believe it reduces the debugging headache in a long run.
# Conflicts: # packages/text-annotator-react/src/TextAnnotator.tsx # packages/text-annotator/src/SelectionHandler.ts
# See - sindresorhus/debounce#8. The debounced function has a strict context comparison that breaks when applied in the resize listener
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts
# Conflicts: # packages/text-annotator/src/TextAnnotator.ts
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts # packages/text-annotator/src/TextAnnotator.ts # packages/text-annotator/src/highlight/baseRenderer.ts
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts # packages/text-annotator/src/utils/index.ts
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts
# Conflicts: # packages/text-annotator/src/utils/index.ts
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts # packages/text-annotator/src/TextAnnotator.ts
# Conflicts: # packages/text-annotator/src/TextAnnotator.ts
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts
# Conflicts: # packages/text-annotator/src/SelectionHandler.ts
# Conflicts: # package-lock.json # packages/text-annotator-react/package.json
# Conflicts: # package-lock.json # packages/text-annotator-react/package.json # packages/text-annotator/src/SelectionHandler.ts # packages/text-annotator/src/utils/index.ts
| const selectionHandler = SelectionHandler(container, state, opts); | ||
| const selectionHandler = createSelectionHandler(container, state, opts); | ||
| selectionHandler.setUser(currentUser); | ||
| selectionHandler.setAnnotatingEnabled(opts.annotatingEnabled); |
There was a problem hiding this comment.
This line should be redundant, right? Because the initial state would already come in through the opts?
There was a problem hiding this comment.
This line should be redundant, right?
Technically, yes. This explicit call was added before the createSelectionHandler started receiving the opts object.
2 participants
Issue - #127
Changes Made
I made the
annotatingEnableda dynamic property that can receive values from the external sources via thesetAnnotatingEnabled(following the "API addition: method to change userSelectAction" example)(Soomo Staged, 22.09.25)