-
-
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
Segmentations Settings UI - Phase 1 #1391 #1392
Conversation
…tations # Conflicts: # platform/viewer/cypress/support/commands.js # platform/viewer/package.json
@fedorov, is there a specific route, study, or steps that cause that error for you? What browser are you testing on? I'm unable to reproduce on the latest netlify deploy preview. |
We tried this at a call today, and @swederik, @JamesAPetts and @pieper were able to reproduce this. We were just selecting a study with SEG. I tried again, and was able to reproduce it on the first try with this study: https://deploy-preview-1392--ohif.netlify.com/pwa/viewer/1.3.6.1.4.1.14519.5.2.1.3671.7001.133687106572018334063091507027 |
I get a slightly different stack trace for the same link:
![image](https://user-images.githubusercontent.com/126077/75581309-3a895000-5a37-11ea-9d6c-fcfcd70a86ff.png)
|
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 bulk of these changes are in extensions/*
, so this feels fairly safe to merge. Notably, this removes the seg to cornerstone bridge that used to live in the cornerstone extension. You will need this extension to visualize, but I think that's just fine. We'll have "modes" near future to help us decouple everything bundled in these extensions.
The other core changes that jumped out to me were:
studies
array passed to<Viewport />
studies
andisOpen
passed to<VisiblePanelRight />
- Some core logic added for sorting, finding, caching derived datasets
Note: this PR will likely mess with the consistent DICOM Tag naming @JamesAPetts has worked to achieve. We need a linter to catch these.
Note: We should address the outstanding issue above. I'm curious to see if @JamesAPetts metadata changes have any impact.
PR Checklist
#1391
@mention
a maintainer to request a review