rgab1508 / PixelCraft

A Pixel Art Editor
https://rgab1508.github.io/PixelCraft
MIT License
1.27k stars 78 forks source link

Resolving errors, validating input #39

Open Minater247 opened 3 years ago

Minater247 commented 3 years ago

Hello! It's me from this Reddit comment. Apart from broader issues that were beyond my knowledge on the workings of this project, I fixed the things I could from the Reddit comment:

What I changed:

What I couldn't fix as I'm unfamiliar with the codebase, but wanted to note for the future:

I don't work with GitHub much (this is my first pull request), so please let me know if there's something wrong with how I'm doing this! I'm always trying to learn.

theabbie commented 3 years ago

@Minater247 Thanks for this, we need some time to review these changes.

eagleloid commented 3 years ago

An error in gif.coffee (dependency of gif.js in the index directory) which can break gif saving if it is clicked multiple times before interaction with save dialog

@Minater247 Made a pull request that I think addresses this. Was very interesting. Learned about coffeescript, js overriding, and how deep in the weeds library dependency can get.

I also don't use GitHub that much, but this project looked straight forward enough to dig into...and I didn't want to do my actual project that pays my rent...

Minater247 commented 3 years ago

@theabbie I thought I should notify you - I've made a minor modification to what I had put. I learned from u/CharieBlastX7 that there's a call in gif.js that cancels the current gif rendering - gif.abort. Adding a call to this before calling gif.render means that you no longer get an error when you click the "Save GIF" button twice in quick succession. From the testing I've done, it works as intended. I hadn't intended to work on this part more since @eagleloid already figured out a way to do this, but this was a more concise and built-in way to do it.

Also, when suggesting a change to a pull request, are you supposed to merge the secondary branch before or after noting here? It doesn't show commits to other branches if I don't merge, but if I merge it's there until edited again - so I'm unsure of the proper GitHub etiquette here. Thank you!

theabbie commented 3 years ago

@Minater247 You should make the changes in master branch of your fork, that's where the pull request is made from.