Fix: Clarify separation between plain and comment highlights #58
Conversation
pramodsum
commented
Dec 1, 2017
•
pramodsum
commented
- Using the last annotation to determine whether or not the current annotation is of a valid and enabled type is problematic because because highlight comment annotations may have a plain highlight as the first annotation in the thread, resulting in a false positive for the thread.
- Fixes bug where both highlight and comment buttons were displayed in the create dialog despite only plain highlights being enabled
|
Verified that @pramodsum has signed the CLA. Thanks for the pull request! |
| @@ -79,8 +79,8 @@ class CreateHighlightDialog extends CreateAnnotationDialog { | |||
| constructor(parentEl, config = {}) { | |||
| super(parentEl, config); | |||
|
|
|||
| this.allowHighlight = !!config.allowHighlight || true; | |||
| this.allowComment = !!config.allowComment || true; | |||
| this.allowHighlight = config.allowHighlight || false; | |||
There was a problem hiding this comment.
Fixes bug where both highlight and comment buttons were displayed in the create dialog despite only plain highlights being enabled
| @@ -542,13 +546,9 @@ class Annotator extends EventEmitter { | |||
|
|
|||
| const pageThreads = this.threads[pageNum] || {}; | |||
| Object.keys(pageThreads).forEach((threadID) => { | |||
| const thread = pageThreads[threadID]; | |||
There was a problem hiding this comment.
Annotations with disabled types should be filtered out by this point so this extra check is unecessary
| @@ -79,8 +79,8 @@ class CreateHighlightDialog extends CreateAnnotationDialog { | |||
| constructor(parentEl, config = {}) { | |||
| super(parentEl, config); | |||
|
|
|||
| this.allowHighlight = !!config.allowHighlight || true; | |||
| this.allowComment = !!config.allowComment || true; | |||
| this.allowHighlight = config.allowHighlight || false; | |||
There was a problem hiding this comment.
Also @JustinHoldstock thoughts on disabling these buttons by default? It feels weird to enable buttons if no config is passed in, however I guess that could result in an empty dialog showing up if nothing is passed in...
There was a problem hiding this comment.
IMO Things should be enabled by default, if they're necessary for preventing developer confusion, and selectively disabled
There was a problem hiding this comment.
Since this is more of a safety check (we make sure to never create a CreateHighlightDialog if !docAnnotator.plainHighlightEnabled && !docAnnotator.commentHighlightEnabled, this is more just a safety net.
| */ | ||
| export function getLastAnnotation(annotations) { | ||
| const numAnnotations = Object.keys(annotations).length; | ||
| const lastAnnotationID = Object.keys(annotations)[numAnnotations - 1]; |
There was a problem hiding this comment.
can you bail out early if there are no annotations?
There was a problem hiding this comment.
Not sure if it's necessary in this or the getFirstAnnotation() method since neither are costly calls. Curious as to the reason why we should be bailing out early?
