-
Notifications
You must be signed in to change notification settings - Fork 78
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
base: master
Are you sure you want to change the base?
Conversation
@Minater247 Thanks for this, we need some time to review these changes. |
@Minater247 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... |
Add gif.abort implementation to cancel previous gif rendering before rendering new gif
@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 - 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 You should make the changes in master branch of your fork, that's where the pull request is made from. |
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.