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 - 194: ROI class based color settings #227

Merged
merged 16 commits into from
Aug 29, 2024

Conversation

pedrokohler
Copy link
Collaborator

Adding the ability to edit ROI colors based on their categories and types. Solves #194.

Copy link

Visit the preview URL for this PR (updated for commit c4213c3):

https://idc-external-006--idc-slim-preview-bhe6cro3.web.app

(expires Thu, 22 Aug 2024 17:05:48 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 88aacecd98ba54d2f9c8d201a9444e43d1ad8307

@fedorov
Copy link
Member

fedorov commented Aug 19, 2024

Looks great testing with https://healthcare.googleapis.com/v1/projects/idc-dev-etl/locations/us-central1/datasets/idc/dicomStores/annotated_rms-original-and-annotations-wc-temp-20240122/dicomWeb (specifically, RMS2455 is a good example).

Few suggestions:

  • Can you populate default colors to allow differentiation between different types?
  • It is nice that you show SCT codes in tooltips for types - can you do the same for categories?
    image

@pedrokohler
Copy link
Collaborator Author

@fedorov

  • It is nice that you show SCT codes in tooltips for types - can you do the same for categories?

Isn't it showing on your end? For me it shows

image

Or do you mean something else?

Can you populate default colors to allow differentiation between different types?

I'll work on this.

@fedorov
Copy link
Member

fedorov commented Aug 20, 2024

Isn't it showing on your end? For me it shows

Indeed, it does show! I guess I was confused by the colors. Why is category black and type green-blue? I think all should be black

@pedrokohler pedrokohler force-pushed the feat-194-sr-class-based-color-settings branch from 79cda95 to 50f2475 Compare August 28, 2024 15:05
@pedrokohler
Copy link
Collaborator Author

@fedorov you can check the changes you required here

https://idc-external-006.web.app/

@fedorov
Copy link
Member

fedorov commented Aug 28, 2024

@pedrokohler this is perfect, great work! Good to go from my perspective.

Copy link
Collaborator

@igoroctaviano igoroctaviano left a comment

Choose a reason for hiding this comment

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

First iteration, few questions!

src/components/AnnotationCategoryList.tsx Show resolved Hide resolved
src/components/AnnotationGroupItem.tsx Show resolved Hide resolved
src/components/SlideViewer.tsx Outdated Show resolved Hide resolved
src/components/SlideViewer.tsx Outdated Show resolved Hide resolved
src/components/SlideViewer.tsx Outdated Show resolved Hide resolved
@igoroctaviano
Copy link
Collaborator

@pedrokohler pedrokohler force-pushed the feat-194-sr-class-based-color-settings branch from a6474dc to 753162e Compare August 29, 2024 12:43
Copy link
Collaborator

@igoroctaviano igoroctaviano left a comment

Choose a reason for hiding this comment

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

Almost there! I notifed the following issue:

  • Open the study
  • Don't activate any ROI
  • Go to the categories section but don't select any
  • Change the color of one of the categories without activating
  • The annotation is not visible but the category check is not checked

@pedrokohler pedrokohler force-pushed the feat-194-sr-class-based-color-settings branch from 3bbbc61 to 125122e Compare August 29, 2024 13:22
Copy link
Collaborator

@igoroctaviano igoroctaviano left a comment

Choose a reason for hiding this comment

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

LGTM

@igoroctaviano igoroctaviano merged commit 648ae73 into master Aug 29, 2024
5 checks passed
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