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

Update example_json_input.html #39

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

Conversation

balamuruganky
Copy link

loadJSON function reading the JSON file now.

loadJSON function reading the JSON file now.
Copied json with peaks from the other example.
svg set to "min-height: 700px"
Removed the unimplemented functionalities.
loadJSON function modified to load the local JSON file.
@balamuruganky
Copy link
Author

@wcjohns : I have fixed few issues opening the local file to plot the spectrum. Please consider to merge. Thanks.

@balamuruganky
Copy link
Author

Fixed compilation issues in node bindings.

@wcjohns
Copy link
Collaborator

wcjohns commented Feb 20, 2025

Thank you for this @balamuruganky!

I had not built the node.js bindings in a while, so I apologize that some of that code had rotted, and it took you some time to fix it.

Did you manage to get things compiling for your needs?
Is there any functionality or anything you are missing? I only know of a few people/apps using this node binding, and all for basically just displaying spectra (I have never used it myself, other than to get it working).

Building the node bindings is sometimes a challenge; I should probably add this to the CI/CD stuff to help avoid future rot (we somewhat recently added Python and C binding for this reason).

While looking around, I noticed a few things (all due to my neglect of this binding):

  • Requiring boost is maybe vestigial, and I think can be removed to make compiling easier. Do you see any issue if I remove anything to do with boost? I think this would mostly then remove the need for the compile script - would you also then be okay with us removing linux_build.sh and essentially copy its commands into the README? I'm not sure that the script will work in all Linux environments, so it may help reduce confusion/maintenance, and no longer having boost should hopefully mitigate the need for it.
  • I like the being able to select the JSON file for the example HTML - this was a nice idea! Would you be okay if I add back in the default loading of JSON, but keep your added ability to select a different JSON through a file picker? The default initial XMLHttpRequest loading (which I think only works in FireFox, if you are opening the HTML from file, and not a server) was requested by someone so they could see how to integrate in with their web-app, so I would a little bit like to keep it around, even though its fairly trivial.
  • I have a preference to keep the CMake install paths not muck with the source directory; how I had it before (relative to build directory) also wasn't great - how about if I change install path to be a relative path (SpecUtilsNode?) so this way it will be a bit more idiomatic and use CMAKE_INSTALL_PREFIX?
  • Having a minimum height for the chart in CSS will mess up places where the chart is used - is it okay if I move this into the HTML file?
  • I need to cleanup the d3_resources directory...

Again, thank you for these improvements and fixes!
-will

@balamuruganky
Copy link
Author

balamuruganky commented Feb 20, 2025

Hi @wcjohns,

Thank you for your comments and I agree all your thoughts on the changes, I have made.
Many thanks for your work on the spectrum files, it is very helpful for the scientific community.

  • As you mentioned, boost is not neccessary to have to compile the node bindings. Please remove the linux script as you wish
  • File picker has been added because the JSON doesn't load other than Firefox. Please add the default JSON loading in case of firefox browser and keep file picker for other browsers.
  • SpecUtilsNode is the better install path name.
  • Sure. It is a good idea to have it in the HTML file without affecting other functionalities.
  • When I get some time, I will add features to SpectrumD3.js and share you the changes.

Thanks again for your nice work!!!

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