-
Notifications
You must be signed in to change notification settings - Fork 116
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
feat(annotations): Toggle region annotation mode #1192
Conversation
src/lib/AnnotationControls.ts
Outdated
return; | ||
} | ||
|
||
switch (this.currentActiveControl) { |
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.
I wonder if the switch is needed at all. Should it just blindly run through all the update*Button
methods each time? For instance if the click region and then click highlight, we would need region and highlight to update respectively
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.
I wonder if the switch is needed at all. Should it just blindly run through all the
update*Button
methods each time? For instance if the click region and then click highlight, we would need region and highlight to update respectively
This is deactivateCurrentControl()
, which only deactivate the current control. Update next control will happen in handle*Click
.
src/lib/AnnotationControls.ts
Outdated
* Region comment button click handler | ||
*/ | ||
private handleRegionClick = (onRegionClick: RegionHandler) => (event: MouseEvent): void => { | ||
const activeControl = this.currentActiveControl; |
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 call to this.deactivateCurrentControl() seems unnecessary. It seems like this method's responsibility should be (and all handle([Region|Highlight|Draw])Click
for that matter):
- update
currentActiveControl
- update all buttons states accordingly
- callback to Viewer with the type toggled
so maybe something like this
this.currentActiveControl = this.currentActiveControl === 'region' ? 'none' : 'region';
this.controlsMap.keys().forEach(updateButton => updateButton());
onModeClick('region');
Thoughts?
@@ -93,16 +95,16 @@ export default class AnnotationControls { | |||
private handleFullscreenExit = (): void => this.handleFullscreenChange(false); | |||
|
|||
/** | |||
* Deactive current control button | |||
* Deactivate current control button |
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.
Update method description?
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.
Update method description?
I think it's the same meaning 😂
No description provided.