Guiders-JS icon indicating copy to clipboard operation
Guiders-JS copied to clipboard

Made a handful of changes and a couple of bug fixes

Open majorcode opened this issue 11 years ago • 2 comments

Hello. First I want to thank you for for this library. Especially for the update that moves the guiders on browser resize. That change landed as I was about to write it myself.

This is my first open source commit. I had been working for the federal government, in isolation, for 12 years. Please go easy on me. Also, I apologize for letting so many changes pool up. I'm a recent convert from Subversion. And this is the fist chance I've had to contribute code back to you.

I carefully made comments for each of the 10 commits. But the complete, unified diff of all commits doesn't really tell the story as I intended. Should these have been 10 separate pull requests?

I look forward to hearing your feedback.

majorcode avatar Feb 19 '14 05:02 majorcode

Thanks Steve. They should have been logically separate pull requests, probably not 10 but not 1 either. It's not that big a deal though.

When I get some time I intend to look through all the pull requests including this and merge a bunch in.

Why did you rename dehighlightElement to removeHighlight?

Jeff Pickhardt [email protected]

On Tue, Feb 18, 2014 at 9:47 PM, Steve Farmer [email protected]:

Hello. First I want to thank you for for this library. Especially for the update that moves the guiders on browser resize. That change landed as I was about to write it myself.

This is my first open source commit. I had been working for the federal government, in isolation, for 12 years. Please go easy on me. Also, I apologize for letting so many changes pool up. I'm a recent convert from Subversion. And this is the fist chance I've had to contribute code back to you.

I carefully made comments for each of the 10 commits. But the complete, unified diff of all commits doesn't really tell the story as I intended. Should these have been 10 separate pull requests?

I look forward to hearing your feedback.

You can merge this Pull Request by running

git pull https://github.com/majorcode/Guiders-JS dev

Or view, comment on, or merge it at:

https://github.com/jeff-optimizely/Guiders-JS/pull/116 Commit Summary

  • Version 2.0.0 / Merge remote-tracking branch 'origin/dev'
  • Version 2.0.0 / Merge remote-tracking branch 'origin/dev'
  • move component.json to bower.json
  • Merge pull request #111 from aub/master
  • Reduce places where the _guiders array is accessed directly by calling get() or getCurrentGuider() to find guiders.
  • If next() is called on a guider that should be skipped, the return value from next() should be the result from the recursive call to next().
  • Use a counter instead of a random number to automatically assign guider IDs.
  • Check for myGuider === null in _attach since typeof null is 'object'.
  • When adding a guider after another and setting its prev ID, reciprocate by setting the previous guider's next ID.
  • Add events to observe guider prev/next navigation.
  • Since we de-highlight guider.highlight, we should be highlighting it instead of guider.attachTo.
  • Reset _currentGuiderID to null when all guiders are hidden.
  • Replace _dehighlightElement() with _removeHighlight() and call it in key places.
  • Add descriptions for guider options that were not listed in the doc.

File Changes

  • M README.mdhttps://github.com/jeff-optimizely/Guiders-JS/pull/116/files#diff-0(4)
  • R bower.jsonhttps://github.com/jeff-optimizely/Guiders-JS/pull/116/files#diff-1(0)
  • M guiders.jshttps://github.com/jeff-optimizely/Guiders-JS/pull/116/files#diff-2(68)

Patch Links:

  • https://github.com/jeff-optimizely/Guiders-JS/pull/116.patch
  • https://github.com/jeff-optimizely/Guiders-JS/pull/116.diff

Reply to this email directly or view it on GitHubhttps://github.com/jeff-optimizely/Guiders-JS/pull/116 .

pickhardt avatar Feb 20 '14 07:02 pickhardt

Hi, Jeff,

About deHighlightElement: In db7545c I rearranged places where the highlight was removed to be sure to cover some other cases. I realized that the most defensive thing to do was remove the highlight from anything with that class. So I thought a new name was best. And, since this is in internal function, the API remains unchanged.

Steve

majorcode avatar Feb 20 '14 13:02 majorcode