swsoyee / r3dmol

🧬 An R package for visualizing molecular data in 3D
https://swsoyee.github.io/r3dmol/
Other
88 stars 4 forks source link

Improving existing functions #9

Closed BradyAJohnston closed 3 years ago

BradyAJohnston commented 3 years ago

I've worked on a couple of things here:

To add the speed option, I had to change the actual 3dmol.js library. I did this manually, including adding to the *.min.js libraries. I figure this is bad practice, but I didn't know how to go about it properly. The actual differences between the files are very minor (only 5 extra characters in the *.min.js files) but because it is on a single line it shows it as all different.

The actual differences are as follows:

Original

this.spin=function(e){if(clearInterval($e),void 0===e&&(e="y"),"boolean"==typeof e){if(!e)return;e="y"}Array.isArray(e)&&(e={x:e[0],y:e[1],z:e[2]});var t=this;$e=setInterval(function(){t.rotate(1,e)},25)};

Changed

this.spin=function(e,s){if(clearInterval($e),void 0===e&&(e="y"),"boolean"==typeof e){if(!e)return;e="y"}Array.isArray(e)&&(e={x:e[0],y:e[1],z:e[2]});var t=this;$e=setInterval(function(){t.rotate(1*s,e)},25)};
swsoyee commented 3 years ago

@BradyAJohnston , Thanks! I haven't review the code yet, so here just is a quick reply.

To add the speed option, I had to change the actual 3dmol.js library.

The speed option of spin is very cool! And this is a very important feature, so I think it would better to implement in an independent Pull Request.

Another thing is that how about considering submit a PR to the original repository? So we don't need to pay too much efforts to solves those conflicts when we update the JavaScript library in {r3dmol}.

BradyAJohnston commented 3 years ago

Speed option for this.spin() is now merged into master of 3Dmol.js

https://github.com/3dmol/3Dmol.js/pull/503#issue-574460861

swsoyee commented 3 years ago

Speed option for this.spin() is now merged into master of 3Dmol.js

3dmol/3Dmol.js#503 (comment)

Thanks, but it would be better to split the enhancement of m_spin to another PR. You could create an issue about this feature enhancement. Before that, It's also need to upgrade 3dmol.js in {r3dmol} to the latest version (now it's v1.5.4). I will upgrade the dependency at soon as possible, or an upgrade PR is also welcomed!