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

Fixed define check #4

Closed
wants to merge 2 commits into from
Closed

Fixed define check #4

wants to merge 2 commits into from

Conversation

operatorjen
Copy link
Contributor

This was breaking on chatspaces, but this fix works!

This was breaking on chatspaces, but this fix works!
@operatorjen
Copy link
Contributor Author

second commit relates to a dom exception that was being created because width/height were set to 0 even though they were already set on https://github.com/sole/Animated_GIF/blob/master/src/Animated_GIF.js#L9

since width/height are already set in the scope, it probably doesn't need to be passed in through addFrame as arguments for setSize. this also got rid of my dom exception error on chatspaces.

this.setSize = function(w, h) {
width = w;
height = h;
this.setSize = function() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why would you remove the parameters for this function whose purpose is to set the width and height - it either is an issue of the API not being clear or another issue that throws the DOM exception you're getting somewhere else. Maybe you're setting the width and height to 0 inadvertently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea - I moved the gumhelper calls to a factory in Angular and it kept throwing an undefined with the way it was set before - when I made this change it was happy. You could try changing the version back to yours locally for chatspaces in bower to see it behave

@sole
Copy link
Owner

sole commented Nov 18, 2013

I'm not going to merge it because you're changing the API (for bad), but I'm going to port the proper define statement, have a look at chatspaces to ensure the GIF part is properly initialised, and review the API to ensure it's clearly documented too. My bad for not doing so before :-(

@sole sole closed this Jan 11, 2014
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.

None yet

2 participants