feat(Tools): add Eraser Tool #806
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Thank you so much for this PR. It is a nice feature to get in. A few overall things for this PR...
|
| continue; | ||
| } | ||
|
|
||
| const annotations = state.getAnnotations(toolName, viewportId); |
There was a problem hiding this comment.
I think here we need element instead of viewportId
| const vp = this.renderingEngine.getViewport(viewportId); | ||
|
|
||
| const addEventListenerForAnnotationRemoved = () => { | ||
| element.addEventListener(csToolsEvents.ANNOTATION_REMOVED, () => { |
There was a problem hiding this comment.
I think here instead of adding the listener to element, eventTarget should be used.
|
@ramonemiliani93 it looks good so far. Perhaps make the suggested changes, push and then we can take this out of draft and go from there. Thanks. |
# Conflicts: # packages/tools/src/index.ts
|
Changes done @jbocce 👌 Let me know If I need to add/update something 💪 |
@ramonemiliani93 thank you for the updates. However the unit test you added is not passing. Does it pass for you? Also I was hoping you would have added the eraser tool to the stack annotation tools example as I had requested. Without these two items it is difficult to test your code and complete the PR. If you could please update accordingly then that would be very much appreciated. |
|
Sorry, @jbocce! It seems like I did not push the changes for the stack annotation tools example. Regarding the test, I have yet to figure out why it is not capturing the event (partly the reason why I opened the PR in draft). If you could point me in the right direction on how to figure out why the |
| this.DOMElements = []; | ||
|
|
||
| this.stackToolGroup = ToolGroupManager.createToolGroup('stack'); | ||
| this.stackToolGroup.addTool(EraserTool.toolName, {}); |
There was a problem hiding this comment.
We need to add the length tool to the tool group for the stack viewport so that it is picked up in the EraserTool code. So below this line add...
this.stackToolGroup.addTool(LengthTool.toolName, {});
| }); | ||
|
|
||
| addEventListenerForAnnotationRemoved(); | ||
| document.dispatchEvent(evt); |
There was a problem hiding this comment.
Dispatch the event on the viewport element itself. (i.e. element.dispatchEvent(evt))
There was a problem hiding this comment.
The only two changes I needed to have the test pass are suggested below. Please give them a try.
| @@ -118,6 +119,7 @@ const toolsNames = [ | |||
| CobbAngleTool.toolName, | |||
| ArrowAnnotateTool.toolName, | |||
| PlanarFreehandROITool.toolName, | |||
| EraserTool.toolName, | |||
There was a problem hiding this comment.
Thanks for adding the eraser tool to the example. I tried it and was able to delete every annotation type except the probe. Could you please have a look at why this might be? Thanks.
Context
Creates an eraser tool that allows the deletion of annotations.
Changes & Results
What are the effects of this change?
Testing
EraserTool_testfile to run tests on the tool.Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-apito update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment