PixelCraft icon indicating copy to clipboard operation
PixelCraft copied to clipboard

Resolving errors, validating input

Open Minater247 opened this issue 4 years ago • 4 comments

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:

  • Added input validation
  • Commented out "Install PWA" as it didn't have content but caused an error
  • Users can no longer activate the "View frames" button before there are any frames to show
  • Users can no longer save before there are any boards to save

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

  • 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
  • Large numbers tend to cause issues, such as duplicated drawing on the canvas or just no canvas at all
  • The canvas is centered by width, not height - so a tall canvas will have most of the pixels obscured off of the screen. I would reccomend making the canvas centered to whichever is larger to ensure all pixels are visible and editable.

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.

Minater247 avatar Jan 16 '21 12:01 Minater247

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

theabbie avatar Jan 16 '21 12:01 theabbie

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...

eagleloid avatar Jan 17 '21 00:01 eagleloid

@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!

Minater247 avatar Jan 18 '21 06:01 Minater247

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

theabbie avatar Jan 18 '21 06:01 theabbie