search

jmpressjs / jmpress.js

A jQuery plugin to build a website on the infinite canvas
http://jmpressjs.github.com/jmpress.js
1.5k stars 237 forks source link

jmpress property in eventData not working for idle callback #175

Open FagnerMartinsBrack opened 9 years ago

FagnerMartinsBrack commented 9 years ago

This is a fix for https://github.com/jmpressjs/jmpress.js/issues/172#issuecomment-69690296. It uses sinon fake timer to trigger the idle event without having to wait 1,4 seconds.

I know the property is not documented, but this is the only way to test if that piece of code is correct, and since the property is there it should at least work for all callbacks.

https://github.com/web-stories/jmpress.js/commit/f32f962e4c9785e78a9df277d50da2f44bdc14ca

shama commented 9 years ago

No need for $.proxy as javascript has bind but I prefer just creating a reference via var self = this and using that.

Issues with links to commits are not very helpful, if you want to contribute could you please open a PR instead? If not I understand, just fork and do your thing.

FagnerMartinsBrack commented 9 years ago

The phantomJS version that is being used in grunt-contrib-qunit latest is not v2, so it doesnt support Function.prototype.bind. To prevent the use of a polyfill just for tests (and considering the performance and char count are not relevant) I opted for $.proxy.

@shama I seriously dont understand why linked commits are not helpful. I dont care for my name on contributions, so you can easly copy/paste the contents after a proper decision without wasting time analyzing the code (I suppose that is the thing that waste more time right?).

I am sorry if I am being stubborn, but it is really difficult to work in a codebase so inconsistent. I hope you understand, I only want to get things done and this is the best way I found to do it. The same way you don't have time to dedicate to this project right now I don't have time to implement things twice.

I just honestly don't get why that's a problem. I thought linking commits would help (or at least document a problem with a potential solution for future pull requests), but if you don't want that I can stop, just let me know.

shama commented 9 years ago

It's not a problem, fork and do your thing. This project is MIT licensed.

No need to open an issue here with a link for each change you've made on your fork. If you're interested in contributing to this project, send a PR and we'll review. If the codebase is too inconsistent or you don't like our opinions and code then just fork and do your own thing. No worries here.

FagnerMartinsBrack commented 9 years ago

I understand MIT license does not require to mention the changes, my intent is that those changes are actually landed here! The effort to fully maintain a fork with a big codebase are not worth it.

I just presented the original commit in order to show a clear diff due to the code style changes (which would break a PR) and discuss about it. Then someone could in the future land those in the codebase, maybe myself with a PR later or some collaborator with a better time availability.

Don't you think that discussing about a piece of code and it's meaning (like this) would add value for future contributions? In my end I am ok with this because my intent is that this is landed eventually, unless you are not ok with this.

shama commented 9 years ago

Please understand I maintain a lot of projects and only have a limited time available to do so. I attempt to address every issue the best I can. I am not interested in discussing code style and internal things with someone whom has not contributed anything meaningful to the project.

I appreciate your intent to help but don't have time to discuss your opinions related to this project. If you see an issue and want to fix it, send a PR. Otherwise please just fork and do your thing.

FagnerMartinsBrack commented 9 years ago

ok