-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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(segmentation): Enhanced segmentation panel design for TMTV #3988
Conversation
✅ Deploy Preview for ohif-platform-docs canceled.
|
✅ Deploy Preview for ohif-dev canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3988 +/- ##
==========================================
- Coverage 46.23% 44.37% -1.87%
==========================================
Files 78 80 +2
Lines 1276 1334 +58
Branches 312 327 +15
==========================================
+ Hits 590 592 +2
- Misses 548 589 +41
- Partials 138 153 +15 see 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Passing run #3787 ↗︎
Details:
Review all test suite changes for PR #3988 ↗︎ |
extensions/cornerstone-dicom-seg/src/panels/PanelSegmentation.tsx
Outdated
Show resolved
Hide resolved
platform/ui/src/components/SegmentationGroupTable/SegmentationDropDownRow.tsx
Outdated
Show resolved
Hide resolved
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.
see my comments please, great work
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.
can we make the scrollbar outside the header
needs to be half
can you add the new segmetnation to the tmtv
fix this toggle as well please
check the design for folded and the i
location
if we can make the scrollbar starts after the header that would be great
--
AddSgement should not touch the bottom, it has top padding seems like
this is going overflow hidden for some reason
this thing should be centered
probably wrong padding on bottom
Fixes:
2.number box sizing fixed
8.Fixed add segment padding
@sedghi please take a look, we just need to add the tools and fix up panel in tmtv. |
@@ -33,16 +31,7 @@ export function Toolbar({ servicesManager }) { | |||
/> | |||
); | |||
|
|||
return disabled ? ( |
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.
duplicate tooltip bug
</svg> | ||
</div> | ||
</div> | ||
{tooltipContainer && ReactDOM.createPortal(tooltipContent, tooltipContainer)} |
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.
react portals
@@ -95,7 +95,7 @@ const toolbarButtons: Button[] = [ | |||
{ | |||
id: 'Threshold', | |||
icon: 'icon-tool-threshold', | |||
label: 'Eraser', | |||
label: 'Threshold Tool', |
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.
fix tool name
useEffect(() => { | ||
if (isOpen && parentRef.current && tooltipRef.current) { |
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.
Nice!
Can you please add a comment on what is happening for future devs, newX, newY, parent ...
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.
if (!isOpen || !parentRef.current || !tooltipRef.current){
return
}
--> less indentation --> more clear
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.
Getting there,
Patient info has wrong icon
The dropdown for the modal seems like getting in the div (maybe we need portal here as well?)
CleanShot.2024-04-02.at.09.11.27.mp4
Empty Segmentation should have one segment at least, so we should add segment 1 programmatically
CleanShot.2024-04-02.at.09.12.13.mp4
suv peak display text does not appear for some reason below the segment index
CleanShot.2024-04-02.at.09.13.18.mp4
seems like active segment cannot be ch
CleanShot.2024-04-02.at.09.15.09.mp4
anged
…tangleROIStartEndThreshold
…nServiceTypes and SegmentationGroupTable components
…#3988) Co-authored-by: Alireza <[email protected]>
Context
Addresses issue #3790
Changes & Results
Pending
Screenshots
Notes
Most work is complete, remaining piece is to switch out the TMTV panel