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/Suggestion(MeasurementTracking):prevent empty sr description when export #3173

Merged
merged 6 commits into from
Jun 19, 2023

Conversation

dxlin
Copy link
Contributor

@dxlin dxlin commented Feb 14, 2023

Context

Suggestion to prevent empty description for DICOM SR Report export. Prevent and alert when users attempts to save with empty description.

Questions:

  1. Alert currently using browser, would bringing UINotificationService be preferred?
  2. Alternatively could use states to disable the save button; however, ran into issues with react hooks when implementation attempted to turn actions passed to Dialog component to state -> is there any examples of a different approach to this?

Changes & Results

-use _handleFormSubmit when enter is pressed (so same code is ran)
-add empty description check before allow processing within _handleFormSubmit
-if empty, stop event resolution and send alert

Testing

  1. Load Image in Basic Viewer
  2. Select length tool
  3. Create length
  4. Start tracking
  5. Change to Measurements panel (on right panel)
  6. Click "Create Report" button
  7. Press enter with empty description
    -> Should ignore and prevent save
  8. Click save button with empty description
    -> Should ignore and prevent save
  9. Type description, then delete (so description is blank again)
  10. Press enter with empty description
    -> Should ignore and prevent save
  11. Click save button with empty description
    -> Should ignore and prevent save
  12. Type description, click save/press enter
    -> Should allow save routine to proceed

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: Windows 10
  • "Node version: 16.15.1
  • "Browser: Chrome 110.0.5481.96

@netlify
Copy link

netlify bot commented Feb 14, 2023

Deploy Preview for ohif-platform-docs canceled.

Name Link
🔨 Latest commit ee1f61b
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/64905929c5a3230008cd60d2

@netlify
Copy link

netlify bot commented Feb 14, 2023

Deploy Preview for ohif-platform-viewer ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-viewer/deploys/640a5d5f8a6b7e0054ba313d
😎 Deploy Preview https://deploy-preview-3173--ohif-platform-viewer.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 settings.

@codecov
Copy link

codecov bot commented Feb 14, 2023

Codecov Report

Merging #3173 (ee1f61b) into master (59cdc4b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3173   +/-   ##
=======================================
  Coverage   38.27%   38.27%           
=======================================
  Files          82       82           
  Lines        1348     1348           
  Branches      303      303           
=======================================
  Hits          516      516           
  Misses        666      666           
  Partials      166      166           

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 59cdc4b...ee1f61b. Read the comment docs.

@sedghi
Copy link
Member

sedghi commented Feb 15, 2023

Thanks
we shouldn't alert, it should just disable the save button

@jenny-hm-lee
Copy link

@sedghi , please take a look at Daniel's latest commit.

@sedghi
Copy link
Member

sedghi commented Mar 9, 2023

The button is not disabled

image

@dxlin
Copy link
Contributor Author

dxlin commented Mar 9, 2023

The upload function is disabled when description is empty as noted in the tests; the button itself is not in the disabled state.

As noted in point 2 of my description, I ran into issues with hooks when attempted to implement via state and would appreciate guidance/examples if this is the preferred method and has been implemented elsewhere.

@sedghi
Copy link
Member

sedghi commented Mar 9, 2023

The mouse hover should show a disable icon and also button should be disabled too look at the measurement panel when there is no measurements

image

@dxlin
Copy link
Contributor Author

dxlin commented Mar 9, 2023

The measurement button isn't based off the dialog component and states can be passed to it when changed. The dialog however sets the state of the buttons at time of creation via the 'actions' array, which I am struggling to modify on the fly.

My attempts revolved around modifying this 'actions' array to pass a disabled state for save button if description is empty; however, that's when I ran into hook issues.

@jenny-hm-lee
Copy link

@sedghi ,
@dxlin had trouble disabling the save button on the dialog component. The measurement panel is not a dialog component, so he could not re-use the example. Specifically, on the dialog component, he ran into "issues with react hooks when implementation attempted to turn actions passed to Dialog component to state".

Currently, when the report name is empty and user clicks "save", nothing is saved. Let me know if this is reasonable for you. It's ok for us.

@sedghi sedghi changed the base branch from v3-stable to master June 19, 2023 13:33
@netlify
Copy link

netlify bot commented Jun 19, 2023

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/649065b4033576742e68d709
😎 Deploy Preview https://deploy-preview-3173--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 settings.

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