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

Resolving errors, validating input #39

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

Conversation

Minater247
Copy link

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 Minater247 changed the title Fixing the issues I could from Reddit Resolving errors, validating input Jan 16, 2021
@theabbie
Copy link
Collaborator

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

@eagleloid
Copy link
Contributor

eagleloid commented Jan 17, 2021

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 and others added 2 commits January 17, 2021 21:15
Add gif.abort implementation to cancel previous gif rendering before rendering new gif
@Minater247
Copy link
Author

Minater247 commented Jan 18, 2021

@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
Copy link
Collaborator

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

@rgab1508 rgab1508 added the In Review We are looking into it label Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Review We are looking into it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants