search

peduarte / wallop

:no_entry: currently unmaintained :no_entry: A minimal JS library for showing & hiding things
1.1k stars 79 forks source link

Add update method #70

Closed honzabilek4 closed 8 years ago

honzabilek4 commented 8 years ago

Related to #69 I've added an update method which sets correct state of global variables after changing slider content. Can you do a quick review? Isn't against the wallop philosophy? I was also wondering about calling the method in the upper initialization part to save some lines of code. (but the name should be slightly different then)

peduarte commented 8 years ago

@honzabilek4 hey, thanks for this.

Couple of comments:

  1. This PR is now conflicting with master as i've since merged other PR's
  2. I don't like the repetition that is happening there currently.

What I mean is that your update method is running the same things that happens at the start of the script (https://github.com/honzabilek4/wallop/blob/25284f23b267469ec128fa2b6e422cc56538e0d6/js/Wallop.js#L42-L44)

Perhaps it'd be better to have them both rely on the same method?

Also, I'm not sure the name update is good enough... is there a better name? Maybe reset? Don't know, just thinking out loud...

honzabilek4 commented 8 years ago

Yes, that's what I was talking about. Name reset seems more appropriate to me. I will get rid of the repetition and create a new pull request then. Is that okay?

peduarte commented 8 years ago

@honzabilek4 yes that's fine. The CI tests are broken, please make sure you test it the package thoroughly.

I'll review it asap.

Thanks!

🍺

honzabilek4 commented 8 years ago

Hi, I've added a new commit with the fix here instead of creating a new PR. All tests are passing and I've also tested it manually. Everything is working and should be fine. :wink:

peduarte commented 8 years ago

Sweet, I'll take a look!