Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(layout): new layout selector #3923

Merged
merged 11 commits into from
Mar 28, 2024
Merged

feat(layout): new layout selector #3923

merged 11 commits into from
Mar 28, 2024

Conversation

IbrahimCSAE
Copy link
Collaborator

@IbrahimCSAE IbrahimCSAE commented Feb 8, 2024 •
edited

Context

Addresses issue/ feature request #3643

Changes & Results

  • Created new LayoutPreset Component
  • Updated design of Layout Selector

image

Testing

Visit https://deploy-preview-3923--ohif-dev.netlify.app/viewer?StudyInstanceUIDs=2.16.840.1.114362.1.11972228.22789312658.616067305.306.2

And test out the different presets.

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • OS: Linux Ubuntu
  • Browser: Chrome

Issues

  • Switching from a 3D hanging protocol to none 3D one causes the entire browser window to go black
  • Sometimes switching to a 3D hanging protocol will not render anything.
  • Toolbar is not visible if the selected viewport is the 3D one, select a non 3D viewport to make the toolbar re-appear, this is problematic if you are in the 3D only hanging protocol.

@netlify Netlify
Copy link

netlify bot commented Feb 8, 2024 •
edited

✅ Deploy Preview for ohif-dev canceled.

Name Link
🔨 Latest commit b9cad5c
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/6605799f6e0148000813e1cc

@netlify Netlify
Copy link

netlify bot commented Feb 8, 2024 •
edited

✅ Deploy Preview for ohif-platform-docs canceled.

Name Link
🔨 Latest commit b9cad5c
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/6605799f3b54ec0008df9fc2

@IbrahimCSAE
Copy link
Collaborator Author

IbrahimCSAE commented Feb 8, 2024 •
edited

Just need to add 1 missing icon from Dan, and then there's these two issues I noticed that are mentioned in the pull, no idea what the cause is yet.

I can notice some of the issues happening because of something in

Viewers/extensions/cornerstone/src/init.tsx

Line 186 in 306bcea

hangingProtocolService.subscribe(

the presentations not being defined, maybe the calls are happening in the wrong order.

@sedghi

@codecov Codecov
Copy link

codecov bot commented Feb 8, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.41%. Comparing base (8a335bd) to head (b9cad5c).
Report is 266 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3923      +/-   ##
==========================================
- Coverage   46.23%   44.41%   -1.83%     
==========================================
  Files          78       80       +2     
  Lines        1276     1333      +57     
  Branches      312      327      +15     
==========================================
+ Hits          590      592       +2     
- Misses        548      588      +40     
- Partials      138      153      +15

see 9 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f610b0...b9cad5c. Read the comment docs.

@cypress Cypress
Copy link

cypress bot commented Feb 8, 2024 •
edited

Passing run #3774 ↗︎

0 43 0 0 Flakiness 0

Details:

fix build
Project: Viewers Commit: b9cad5c304
Status: Passed Duration: 05:52 💡
Started: Mar 28, 2024 2:19 PM Ended: Mar 28, 2024 2:25 PM

Review all test suite changes for PR #3923 ↗︎

@IbrahimCSAE IbrahimCSAE changed the title fea(layout): new layout selector feat(layout): new layout selector Feb 8, 2024
sedghi
sedghi reviewed Feb 9, 2024
View reviewed changes
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried it yet, since I know you know there are bugs, but great first draft thanks

@IbrahimCSAE
Copy link
Collaborator Author

@sedghi I addressed the comments, I can look more into the bugs that are happening on Monday.

sedghi
sedghi requested changes Feb 27, 2024
View reviewed changes
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure this is not the design

CleanShot 2024-02-27 at 09 01 18

We should probably call it Layouts or something since it is not grid any more, ask Dan

CleanShot 2024-02-27 at 09 02 01

Probably we don't need MPR button anymore

CleanShot 2024-02-27 at 09 02 53

since we moved it to layout

The other 3D rotate icon is too small too, it is something related to the new iconds

CleanShot 2024-02-27 at 09 03 19

The icon should be first if I remember correctly and also the size is too small

sedghi
sedghi reviewed Mar 27, 2024
View reviewed changes
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed based on the latest toolbar changes

Can you please take care of the followings?

The grid layout remains there after layout change (not new bug, but we should fix it)

CleanShot.2024-03-27.at.17.22.57.mp4

For some reason the last column does not appear

CleanShot 2024-03-27 at 17 24 28@2x

@IbrahimCSAE IbrahimCSAE requested a review from sedghi March 28, 2024 04:56
@IbrahimCSAE
Copy link
Collaborator Author

before you merge I think you need to add your toolbar changes to the modes, I only see trackballrotate in basic viewer, and the crosshairs don't disable as expected in segmentation mode but works good in basic viewer.

sedghi
sedghi approved these changes Mar 28, 2024
View reviewed changes
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks amazing, great job

@sedghi sedghi merged commit 617043f into master Mar 28, 2024
10 of 11 checks passed
@rmasad
Copy link

rmasad commented Mar 28, 2024

Testing this: https://deploy-preview-3923--ohif-dev.netlify.app/viewer?StudyInstanceUIDs=2.16.840.1.114362.1.11972228.22789312658.616067305.306.2

Great work!

Just one detail, when you click on some 3D layout and then put a normal layout, it goes black

@sedghi
Copy link
Member

sedghi commented Mar 28, 2024

@rmasad Yeah i know that and working on it right now

@rmasad
Copy link

rmasad commented Mar 28, 2024

thanks @sedghi!

edcheyjr pushed a commit to edcheyjr/Viewers that referenced this pull request Apr 7, 2024
@IbrahimCSAE @edcheyjr
feat(layout): new layout selector with 3D volume rendering (OHIF#3923)
0c09972
thanh-nguyen-dang pushed a commit to uc-cdis/Viewers that referenced this pull request May 1, 2024
@IbrahimCSAE @thanh-nguyen-dang
feat(layout): new layout selector with 3D volume rendering (OHIF#3923)
09806ba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pavelpisarev pavelpisarev pavelpisarev left review comments

@sedghi sedghi sedghi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@IbrahimCSAE @rmasad @sedghi @pavelpisarev