-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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: reference lines #1482
feat: reference lines #1482
Conversation
@dannyrb, can you try this, please? |
Codecov Report
@@ Coverage Diff @@
## master #1482 +/- ##
=======================================
Coverage 11.73% 11.73%
=======================================
Files 276 276
Lines 7088 7088
Branches 1326 1326
=======================================
Hits 832 832
Misses 5075 5075
Partials 1181 1181
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
@swederik and @JamesAPetts, I would also like to hear your opinion. |
extensions/cornerstone/src/init.js
Outdated
@@ -275,3 +287,228 @@ const _connectToolsToMeasurementService = measurementService => { | |||
} | |||
); | |||
}; | |||
|
|||
const _enableReferenceLines = () => { | |||
const waitForImageRendered = enabledElement => |
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.
Is this your "null safety", but for making sure a cornerstoneElement has rendered an image?
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.
Exact. This value can be null If you choose a 2 x 1 viewport layout and change the image index of the first viewport before the image of the second viewport is rendered.
extensions/cornerstone/src/init.js
Outdated
}); | ||
|
||
const renderReferenceLines = ({ detail: { enabledElement } }) => { | ||
const { activeViewportIndex } = window.store.getState().viewports; |
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.
This is the last window.store
reference (if the other comments are addressed). I think we might have to keep it for now, but we should create a follow-up issue where we find a better way to provide this information to extensions.
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.
Yes I agree.
Hi there, it there a reason this feature seems abandoned ? Best regards Salim |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
any update on this? would really like to get this feature |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Please merge this request. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
any update on this? would really like to get this feature |
@arthur-miller If you have working code. please share |
base has changed, read more here #3477 |
reference lines are already in master branch for v3 |
Hello! I hope I'm not bothering you guys.
Here is my reference lines implementation, following @dannyrb 's guidelines (#619).
I will be very happy if you can share some of your thoughts on this with me.
Goals:
renderActiveReferenceLine
.Known limitations:
PR Checklist
@mention
a maintainer to request a review