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

Refactor Scribl to an npm module #35

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

Conversation

multimeric
Copy link

Warning, this is quite a large PR:

  • Rewrote javascript into roughly ES2015 level JS
  • Used import and export statements to structure the library
  • Added webpack settings for compiling the library into UMD
  • Added package.json with relevant metadata for NPM publishing
  • Removed the libs copied from Dalliance, and instead added them as npm dependencies
  • Removed the implicit dependency on jQuery for event handling
  • Fixed some serious code issues (using global variables, among other things)
  • Added a new development section to the readme
  • Removed the use of a custom Class construct, and instead used ES2015 classes and inheritance
  • Fixed some minor JSdoc issues (such as the use of Int and Float instead of number)
  • Minor style changes (mostly fixing indentation)

@multimeric
Copy link
Author

I've tested the examples html files, which all work fine. However I'm leaving this as a draft until I have checked that this works correctly when it's imported as a module (as opposed to being used in the global scope, which the examples do)

@multimeric multimeric marked this pull request as ready for review October 3, 2019 04:01
@multimeric
Copy link
Author

I ended up writing my own visualisation library for this, but I still did a heap of work refactoring Scribl so it might be useful to merge this, if you get a chance to review the massive amount of changes.

From my brief testing, it works for all the examples that worked in the original Scribl, and it worked in my React.js tests as well. I can't tell if I haven't broken some edge case, however, because there isn't a unit testing suite.

@chmille4
Copy link
Owner

chmille4 commented Oct 8, 2019

I will try to review and merge though it will probably take me some time. Thank for this.

@multimeric
Copy link
Author

Great, let me know if you have any questions about the changes I've made.

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