Chore: Store threads.annotations as an object by annotationID#5
Chore: Store threads.annotations as an object by annotationID#5pramodsum merged 8 commits intobox:masterfrom pramodsum:refactor
Conversation
pramodsum
force-pushed
the
master
branch
19 times, most recently
from
b5efd2a to
3521db1
Compare
JustinHoldstock
left a comment
JustinHoldstock
left a comment
There was a problem hiding this comment.
I know it's painful to see all the getFirstAnnotation() comments, but I'm marking them as such so it's easier for you to find them.
| this.bindCustomListenersOnThread(thread); | ||
|
|
||
| this.emit(ANNOTATOR_EVENT.fetch); | ||
| if (this.modeControllers[firstAnnotation.type]) { |
There was a problem hiding this comment.
can you assign controller above the if, instead of doing a double lookup?
| if (annotations.length > 0) { | ||
| threadParams.threadID = annotations[0].threadID; | ||
| threadParams.threadNumber = annotations[0].threadNumber; | ||
| if (Object.keys(annotations).length > 0) { |
There was a problem hiding this comment.
You could circumvent having to check the length of annotations every time by having annotatorUtil.getFirstAnnotation() return null of the list is empty. Then you could just check
if (firstAnnotation)...
|
|
||
| if (annotations.length > 0) { | ||
| this.assignDrawingLabel(annotations[0]); | ||
| if (Object.keys(annotations).length > 0) { |
There was a problem hiding this comment.
See getFirstAnnotation() comment above
| const canDelete = canCommit || (annotations[0].permissions && annotations[0].permissions.can_delete); | ||
| const canCommit = Object.keys(annotations).length === 0; | ||
| const firstAnnotation = annotatorUtil.getFirstAnnotation(annotations); | ||
| const canDelete = canCommit || (firstAnnotation.permissions && firstAnnotation.permissions.can_delete); |
There was a problem hiding this comment.
Will this blow up if first annotation doesn't exist?
| // Determine if highlight buttons or comments dialog will display | ||
| if (annotations.length > 0) { | ||
| this.hasComments = annotations[0].text !== '' || annotations.length > 1; | ||
| if (Object.keys(annotations).length > 0) { |
There was a problem hiding this comment.
See getFirstAnnotation() comment above
| const highlightButtons = this.highlightDialogEl.querySelector(constants.SELECTOR_HIGHLIGHT_BTNS); | ||
| annotatorUtil.hideElement(highlightButtons); | ||
| } else if (annotations[0].permissions && !annotations[0].permissions.can_delete) { | ||
| } else if (firstAnnotation.permissions && !firstAnnotation.permissions.can_delete) { |
There was a problem hiding this comment.
How's this handle no firstAnnotation?
There was a problem hiding this comment.
Added a check in the if statement above to make sure firstAnnotation exists
| if (this.annotations.length && this.annotations[0].permissions && !this.annotations[0].permissions.can_delete) { | ||
| const firstAnnotation = annotatorUtil.getFirstAnnotation(this.annotations); | ||
| if ( | ||
| Object.keys(this.annotations).length && |
There was a problem hiding this comment.
Since first annotation cannot exist if this.annotations is empty, just check for firstAnnotation, and comments on getFirstAnnotation()?
| // Ensures that previously created annotations have the right type | ||
| if (this.annotations.length) { | ||
| if ((this.annotations[0].text !== '' || this.annotations.length > 1) && this.type === TYPES.highlight) { | ||
| if (Object.keys(this.annotations).length) { |
There was a problem hiding this comment.
Above comment on getFirstAnnotation()
| } else { | ||
| this.deleteAnnotation(this.annotations[0].annotationID); | ||
| const firstAnnotation = annotatorUtil.getFirstAnnotation(this.annotations); | ||
| this.deleteAnnotation(firstAnnotation.annotationID); |
There was a problem hiding this comment.
does this behave if no first annotation?
| if (annotations.length > 0) { | ||
| threadParams.threadID = annotations[0].threadID; | ||
| threadParams.threadNumber = annotations[0].threadNumber; | ||
| if (Object.keys(annotations).length > 0) { |
There was a problem hiding this comment.
getFirstAnnotation() comments above
2 participants
Initial PR was box/box-content-preview#395