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(LengthTool): Added digits configuration for Length tool #1407

Merged

Conversation

brunoalvesdefaria
Copy link
Collaborator

@brunoalvesdefaria brunoalvesdefaria commented Jul 26, 2021

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    A new feature which enables the configuration of the amount of digits displayed for the Length tool text box.

  • What is the current behavior? (You can also link to an open issue here)
    The current behavior displays always 2 digits for the Length tool text box.

  • What is the new behavior (if this is a feature change)?
    Now it will be possible to add the digits attribute to the Length tool's configuration object in order to change the amount of digits to be displayed by the measurements' text box.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    No

  • Other information:
    Steps to validade the new behavior:

  1. Open the Netlify preview for this PR;
  2. Open the browser's developer tools;
  3. Change the Length tool configuration from CornerstoneTools' store: cornerstoneTools.store.state.tools[20].configuration.digits = 1;
  4. Select the length tool and draw length measurements.
  5. Note that the amount of digits for the measurements' text boxes have decreased to 1.

@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #1407 (e4748f8) into master (41352cc) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1407   +/-   ##
=======================================
  Coverage   19.86%   19.86%           
=======================================
  Files         286      286           
  Lines       10133    10133           
  Branches     2068     2068           
=======================================
  Hits         2013     2013           
  Misses       6913     6913           
  Partials     1207     1207           
Impacted Files Coverage Δ
src/tools/annotation/LengthTool.js 37.34% <0.00%> (ø)

Continue to review full report at Codecov.

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

@sedghi
Copy link
Member

sedghi commented Aug 18, 2021

@brunoalvesdefaria Since changes are minor, why not adding them to all annotation tools? Otherwise looks great

@brunoalvesdefaria
Copy link
Collaborator Author

@brunoalvesdefaria Since changes are minor, why not adding them to all annotation tools? Otherwise looks great

Thank you for reviewing it @sedghi!
I didn't include this configuration for BaseAnnotationTool because some of them doesn't display numbers and some display numbers that would not make sense to have decimal digits being displayed.
I think it's better to keep the configuration individual for each tool.

@mix3d
Copy link
Contributor

mix3d commented Aug 19, 2021

Throwing in my 2 cents: if it's implemented at the base annotation level, each tool can choose what it needs to display and can optionally allow for configuration if it makes sense.

If you only add it to length, then you have to duplicate code when adding it to the circle annotation, for example.

@brunoalvesdefaria
Copy link
Collaborator Author

Throwing in my 2 cents: if it's implemented at the base annotation level, each tool can choose what it needs to display and can optionally allow for configuration if it makes sense.

If you only add it to length, then you have to duplicate code when adding it to the circle annotation, for example.

I'll summon @swederik to ask his opinion.

We have 3 options:

  1. Keep as is (only for Length tool) and replicate the configuration for other tools that might need it.

  2. Change the BaseAnnotationTool to include digits configuration for all annotation tools (even the ones who doesn't need that, like TextMarkerTool).
    If we do that, we would need to append a constructor block to the BaseAnnotationTool file and include a custom configuration override to add only this digits property (might be polluting the file?).

  3. Do it at the BaseTool level, but it would include digits configuration for all tools (even stack scrolling), which I think it could be confusing. So I think this option is not ideal.

@swederik
Copy link
Member

I wouldn't do it in BaseAnnotationTool because some tools show more than one value. You might want one level of precision for area and another for mean HU/SUV. I know it's a bit ugly to put it in each tool but it's probably the best option in my opinion.

@swederik swederik merged commit 9333b63 into cornerstonejs:master Aug 25, 2021
@dannyrb
Copy link
Member

dannyrb commented Aug 25, 2021

🎉 This PR is included in version 5.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@brunoalvesdefaria brunoalvesdefaria deleted the feat/length-digits-config branch August 25, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants