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

jmpress property in eventData not working for idle callback #175

Open
FagnerMartinsBrack opened this issue Jan 13, 2015 · 6 comments
Open

Comments

@FagnerMartinsBrack
Copy link

This is a fix for #172 (comment). 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.

web-stories@f32f962

@shama
Copy link
Member

shama commented Jan 13, 2015

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
Copy link
Author

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
Copy link
Member

shama commented Jan 13, 2015

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
Copy link
Author

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
Copy link
Member

shama commented Jan 14, 2015

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
Copy link
Author

ok

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

No branches or pull requests

2 participants