-
Notifications
You must be signed in to change notification settings - Fork 5
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
allow hide/show/etc to be fired when selector selects multiple elements #15
Conversation
Heya! Thank you again for taking the time to contribute! I just checked through your code, and here are my comments: I think the style changes are pretty significant. Could I get you to revert those, so they don't get committed in here? (It should be possible to temproarily disable auto-formatting in your editor, if that's what is doing it.) They have also made it quite hard to me to figure out what exactly you have changed 😦 The test seems good to me, and evaluates as negative without the changes you have introduced. Here are a couple of other tests it would be interesting to run:
Once these are done, we can merge and publish 😄 Let me know if you have trouble with any of these, or need some help, and I'll be happy to give more assistance 😄 |
…rely code formatting changes in lines unrelated to the fix for multi-selectors
I understand about the request to revert the purely code formatting changes (I find it next to impossible to read/understand code that is formatted in an "alien" style and I'm sure you're the same way) . I've reverted them and tried to match your formatting on the functional changes. I've also added more unit tests as you suggested. Be sure to look all of my unit tests over very carefully because I still haven't had time to read the QUnit docs to make sure that the tests are testing what I think they are testing...I just copied some of your existing tests and made the changes I think are necessary to test the new functionality. Also, do you have any idea why |
Hey! Thanks for the changes :-) I tried to set up Travis but it isn't quite working yet. (It's hard to get to run tests that deal with visibility in the browser from the command line.) Just ignore it :-) The code looks good, and thanks for all the tests! The last comment I have is that you seem to have used tabs for indentation, whereas the rest of the code uses four spaces. Do you think you could fix this? Then we're ready to merge and I'll put together a release 😄 Would you like to be featured in the README as a contributor? I was thinking of setting this up anyway. What would be your preferred name and link to list? |
Sure, always nice to be recognized for contributions. Use "Paul Biron" and link to my github page. |
Woo! This looks really good 😄 I'll get around to putting an actual release together. Thank you for your time and contributions, and I'll update you here once your name appears in the contributors' section. |
Done :-) I also put up the new release, on here and on npm. |
cool! |
Fixes Issue 14: Hiding multiple elements with a single selector does NOT fire showandtell.hide.
I added 1 new unit to test, but there should be more tests for "multi-selectors", but don't have time at the moment to bone up on QUnit to make sure I'm writing the tests correctly.
All of the existing unit tests that had previously passed continue to pass and the 3 existing unit tests that had previously failed continue to fail.
Also note: I reformatted the code to the style I prefer. Feel free to change it back to your preferred style.