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

fix: Inconsistencies and update the style setting on load for embedded styles from codingValues #4599

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

Conversation

wayfarer3130
Copy link
Contributor

Context

When the codingValues has a site/style value, and an SR is loaded containing annotations labelled with finding/findingSites values, there are inconsistencies with how the styling is applied, or whether it is applied at all.
This also occurred with styling applied in different orders.

Changes & Results

Added some extra styling for the test codingValues provided so that this can be seen.
Changed the updateMeasurement to apply the provided style first, but otherwise use the first style found from: finding, the first findingSite found.
Added support for multiple findingSites names. This allows differentiation between site data for different uses, but doesn't really deal well with multiple sites yet.

Testing

Draw some annotations in basic test mode
Right click annotations, and use the finding/site sub-menus to apply finding or site values.
Note how the new version is order independent - the finding style always takes precedence if there is one, while the previous version last change to be applied got done.
Save an SR
Load the SR again in basic mode

You should see the annotation styles being applied.

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:

@wayfarer3130 wayfarer3130 requested a review from sedghi December 12, 2024 19:52
Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 13f6426
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/6760c0273d05cd000857d4e6
😎 Deploy Preview https://deploy-preview-4599--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.

Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit 13f6426
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/6760c0274e91df0008086c08
😎 Deploy Preview https://deploy-preview-4599--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

cypress bot commented Dec 17, 2024

Viewers    Run #4559

Run Properties:  status check passed Passed #4559  •  git commit 13f6426e9c: Remove color change on highlight
Project Viewers
Branch Review fix/style-for-multiple-findingSites
Run status status check passed Passed #4559
Run duration 02m 22s
Commit git commit 13f6426e9c: Remove color change on highlight
Committer Bill Wallace
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 44
View all changes introduced in this branch ↗︎

@@ -23,28 +23,44 @@ const codingValues = {
'SCT:69536005': {
text: 'Head',
type: 'site',
style: {
color: 'red',
Copy link
Member

Choose a reason for hiding this comment

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

Can we normalize this to RGB or hex instead of text? What shade of red are we referring to, and what is its brightness?

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.

see my only comment please

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.

2 participants