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

#148 Manipulations with DICOM vue #197

Merged
merged 2 commits into from Nov 13, 2017

Conversation

Projects
None yet
6 participants
@vessemer
Copy link
Contributor

vessemer commented Oct 31, 2017

Manipulations over DICOM such as scroll, window width / window centre, etc. series were requested by both #148 and #149. Were implemented by cornerstoneTools. It took a while to distinguish sustainable versions, compatible with this project:

    "cornerstone-core": " 0.13.0",
    "cornerstone-tools": " 0.10.0",
    "jquery": "^3.2.1"

I'd like to separate them from drawing features and made them part of OpenDICOM, still any suggestions are highly appreciated!

Description

In this PR scroll, window width / window centre DICOM manipulators were implemented.

Reference to official issue

This partially addresses #148

Screanshots

Scrolling:

peek 2017-10-31 04-13

WWWC:

peek 2017-10-31 04-14
P.S. there are errors in cornerstone-tools' README should be cornerstoneTools.external not in plural.

CLA

  • I have signed the CLA; if other committers are in the commit history, they have signed the CLA as well
@@ -82,6 +82,8 @@
"vue-style-loader": "^3.0.1",
"vue-template-compiler": "^2.4.2",
"cornerstone-core": " 0.13.0",
"cornerstone-tools": " 0.10.0",
"jquery": "^3.2.1",

This comment has been minimized.

@lamby

lamby Oct 31, 2017

Contributor

There was a question-mark in previous PRs regarding needing jQuery - does that apply here? :)

This comment has been minimized.

@vessemer

vessemer Oct 31, 2017

Contributor

I'll be glad to get rid of jquery, but cornerstoneTools simply does not work with out it

This comment has been minimized.

@louisgv

louisgv Nov 1, 2017

Contributor

Sadly that seems to be the case with cornerstone-tools so we will have to make do with it. I went through the source of cornerstone-tools and their usage of jQuery is somewhat justifiable for complex input event handling. Had they have a more experienced JS contributor, a couple of addEventListener together with more modular project structure could have avoided jQuery.

Also can always do perf test later and create a minimal version of jquery with all the function cornerstone-tools need.

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 3, 2017

there are errors in cornerstone-tools' README should be cornerstoneTools.external not in plural.

Can you link to your PR that fixes this for an extra point? :)

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 3, 2017

Otherwise LGTM, ready to merge ^

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 5, 2017

@vessemer Hey, any update on this? :)

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Nov 6, 2017

Hey, @lamby, was unavailable for a while, there already exists PR for it

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 6, 2017

@vessemer No problem. Should we therefore close this one then..?

@vessemer

This comment has been minimized.

Copy link
Contributor

vessemer commented Nov 6, 2017

Well, I guess that for now, it's better to take cornerstone-tools as is and merely restrict jquery usage by this case only. It'll be beneficial to end up with some minimal version of jquery required by cornerstone-tools or even get rid of it at all, but, perhaps it'll be an issue for the packaging milestone, isn't it?
@louisgv do you mind creating an issue for this?

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 7, 2017

(jquery slim?)

@louisgv louisgv referenced this pull request Nov 8, 2017

Closed

Eliminate/Restrict Jquery use for cornerstone-tools #215

0 of 1 task complete
@louisgv

This comment has been minimized.

Copy link
Contributor

louisgv commented Nov 8, 2017

@vessemer issue created

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 8, 2017

Neat!

@louisgv What do you think here? :)

@louisgv

This comment has been minimized.

Copy link
Contributor

louisgv commented Nov 8, 2017

@lamby Jquery-Slim is still pretty sizable, according to their release note, it's "23.6k vs 30k.".

It would be best if we can use this pipeline: https://github.com/jquery/jquery/blob/master/README.md#how-to-build-your-own-jquery

And create an even slimmer version of jquery for cornerstone-tools. For now I think definitely let's use jquery-slim, restrict its usage, and have #215 tracking progress on the final jquery for corner-stone.

I haven't look closely at the module list of jquery-slim and whether if it has exactly what cornerstone need. But for now, I think we should be good to merge.

EDIT: so jquery-slim excludes -ajax,-effects. The modules I'm most concerned about is -deprecated

jQuery modules: https://github.com/jquery/jquery#modules

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 9, 2017

Pinging @vessemer To get his input :)

@Serhiy-Shekhovtsov

This comment has been minimized.

Copy link
Contributor

Serhiy-Shekhovtsov commented Nov 11, 2017

Guys @lamby, @louisgv, these changes are very important for #148 and #150. In fact, I can't proceed with #148 without it. Can we merge the current request, since we have #215 for "slimmer version of jquery"?
P.S. great job, @vessemer!

@lamby

This comment has been minimized.

Copy link
Contributor

lamby commented Nov 12, 2017

Sure. I'd love to get another pair of eyes on this though.. @isms ? :)

@isms isms merged commit d326360 into drivendataorg:master Nov 13, 2017

2 checks passed

concept-to-clinic/cla @vessemer has signed the CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@isms

This comment has been minimized.

Copy link
Contributor

isms commented Nov 13, 2017

Merged. This is great - thanks! 🎊

@vessemer vessemer deleted the vessemer:148_detect_and_select branch Nov 14, 2017

@swederik

This comment has been minimized.

Copy link

swederik commented Nov 16, 2017

Nice work! Awesome to see Cornerstone added to this project!

Sorry about the issues with the README. For what it's worth, we are working towards dropping jQuery (though it will be slow to remove it from Cornerstone Tools since we are using it all over the place for events).

@isms isms unassigned lamby Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment