search

LeaVerou / multirange

A tiny polyfill for HTML5 multi-handle sliders
http://projects.verou.me/multirange/
MIT License
607 stars 82 forks source link

This pull request fixes four issues #30

Closed jslegers closed 7 years ago

jslegers commented 7 years ago

The following issues are fixed by this pull request :

Indentation errors are also fixed.

jslegers commented 7 years ago

It looks like you broke the indentation. This project (just like all my projects) uses smart tabs for indentation.

The indentation was broken. More specifically, your code for self.multirange = function(input) { ... } didn't indent at all & neither did the if-else statement at the end. I actually fixed that for you, along with the other issues mentioned.

Anway, I added a third commit that changes the indentation style back to smart tabs. It literally took my less than half a minute to change back the indentation style, so I don't get what's the fuzz all about.

Also, in the future please prefer smaller PRs over bigger ones. What happens if I want to merge your fix for one of the issues but not another? By sending huge PRs like that, I can either merge the whole thing on none of it.

I tried to keep the PR as small as possible, but I'm working on a tight schedule here and I needed to get these issues fixed ASAP. Either way, the changes are all pretty small. If you want to undo one or two of them, it shouldn't be much of a hassle.

At least you have a working library now, with AMD support & without duplication of IDs. I'd worry more about my demo not working correctly (or not working at all, as was the case when I first tried it) than whether the code uses smart tabs or two spaces. But maybe it's just me?!

LeaVerou commented 7 years ago

Your last commit switched the indentation back to spaces again.

I went back and forth about merging this and fixing the issues myself, but I would really rather not pollute the commit log with a commit that changes every single line pointlessly. This breaks git blame, and there's absolutely no reason for it. Also, sending a PR that changes every single line because you can't set your editor to tabs and review the diff is disrespectful for the project.

Btw, I notified @rasben whose PR seems to have caused the breakage, since that's much more urgent than any other fixes.

jslegers commented 7 years ago

Fuck this shit. I'll just go my own way with my fork of your project.