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.

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: improve dicom tag browser #4451

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pedrokohler
Copy link
Collaborator

Context

Addresses: #2372

Changes & Results

Collapse and expand items:

image

Deep search:

image

Use slider to navigate between series instances:

image

Testing

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:
  • [] Node version:
  • [] Browser:

Copy link

netlify bot commented Oct 31, 2024

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit 6e87c43
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/6723f5f36135a40008533669
😎 Deploy Preview https://deploy-preview-4451--ohif-platform-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 31, 2024

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 6e87c43
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/6723f5f3e6339500087b7fe5
😎 Deploy Preview https://deploy-preview-4451--ohif-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@pedrokohler
Copy link
Collaborator Author

@fedorov you can test it here: https://deploy-preview-4451--ohif-dev.netlify.app/

@IbrahimCSAE IbrahimCSAE changed the title feat: improve dicom tag browswer feat: improve dicom tag browser Oct 31, 2024
@sedghi
Copy link
Member

sedghi commented Nov 1, 2024

Can you not touch the top header?

CleanShot 2024-11-01 at 09 08 25@2x

If you can afford not changing that we can merge in 3.9, otherwise it needs to go into the queue for design

@sedghi
Copy link
Member

sedghi commented Nov 1, 2024

Also noticed that clicking on the (-) doesn't do anything
CleanShot 2024-11-01 at 09 09 36@2x

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.

You're introducing a new UI library (Ant Design) into OHIF, which doesn't align with our planned migration to shadcn/ui tables. I can't merge this until it uses the shadcn/ui table component.

@pedrokohler
Copy link
Collaborator Author

Also noticed that clicking on the (-) doesn't do anything

It doesn't because you searched for a nested item. In this case it won't. Try without searching and you'll see it works.

Copy link
Collaborator

@IbrahimCSAE IbrahimCSAE left a comment

Choose a reason for hiding this comment

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

I like the new change Pedro, it's much better, but like Alireza said, we are using shadcn for all the new components, look at @ohif/ui-next, and https://ui.shadcn.com/, define the components you need in ui-next using shadcn, then import them in the dicom tag browser to build the desired behavior.

@sedghi
Copy link
Member

sedghi commented Nov 6, 2024

Please revise this PR by tomorrow to include it in 3.9; otherwise, it will be moved to 3.10-beta. Thanks.

@pedrokohler
Copy link
Collaborator Author

Please revise this PR by tomorrow to include it in 3.9; otherwise, it will be moved to 3.10-beta. Thanks.

I have other priorities right now. This PR will probably have to wait a few weeks, unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants