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.

Sign up for GitHub

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

Jump to bottom

Fix Issue 397 #398

Merged
merged 5 commits into from
Oct 19, 2017
Merged

Fix Issue 397 #398

merged 5 commits into from
Oct 19, 2017

Conversation

michib
Copy link
Contributor

@michib michib commented Oct 12, 2017

No description provided.

@michib
Copy link
Contributor Author

michib commented Oct 12, 2017

Fixing #397

vaibhav011286
vaibhav011286 approved these changes Oct 13, 2017
View reviewed changes
Copy link

@vaibhav011286 vaibhav011286 left a comment

Choose a reason for hiding this comment

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

It works

@thedustinsmith
Copy link
Contributor

Thanks for the PR. Sorry for the delay. Would you mind throwing an empty imgData declaration in the block above? Perhaps on line 974? After the move, it isn't being declared. After that this PR looks great. Thanks. For future reference. There's no need to minify and update the version. Since there's usually multiple PR's per release, this causes more conflicts than it helps.

@michib
Copy link
Contributor Author

michib commented Oct 17, 2017

Ok, sorry for the changed version and min.js. I needed it for a local deploy.
I reverted these changes and added imgData = null to the declaration block.

@thedustinsmith
Copy link
Contributor

Not your fault. I put it in the README, but that was more for my own purposes. I can see why that'd be confusing.

Thanks for the PR, I'd like to pull it down and check a few things, but I don't see any reason why this shouldn't be in the next version.

@michib
Copy link
Contributor Author

michib commented Oct 17, 2017

Great, thank you.

It was helpful in the README, so we already could use a fixed version locally.

@thedustinsmith thedustinsmith merged commit 19200f9 into Foliotek:master Oct 19, 2017
@diachedelic diachedelic mentioned this pull request Nov 29, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vaibhav011286 vaibhav011286 vaibhav011286 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michib @thedustinsmith @vaibhav011286 @michaelblumenstein