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

Made a handful of changes and a couple of bug fixes #116

Open
wants to merge 14 commits into
base: dev
Choose a base branch
from

Conversation

majorcode
Copy link

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.

pickhardt and others added 14 commits August 5, 2013 21:25
rename component.json to bower.json
…g get() or getCurrentGuider() to find guiders.

Also, simplify a few guider lookup tests to a simple truth test because the return from get() and getCurrentGuider() are normalized to null when a guider was not found in the _guiders array.
…lue from next() should be the result from the recursive call to next().
…er IDs.

The predictable ID numbering is handy when debugging. And this technique is a tad faster.
…te by setting the previous guider's next ID.
…instead of guider.attachTo.

While it is common to set highlight to the same ID as attachTo, I often attach to one thing but highlight another--like the attachTo's container. Also, must the guider have an attachTo for a highlight to be valid? If not, the test of attachTo is also unnecessary. It's no harm right now, so I left it in.
…key places.

Note that hideAll() was often called before _dehighlightElement(). So it makes sense to ensure that the highlight is cleared in hideAll(). I also remove the highlight before setting the next one, and in the top of show().
@pickhardt
Copy link
Owner

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
pickhardt@gmail.com

On Tue, Feb 18, 2014 at 9:47 PM, Steve Farmer notifications@git.luolix.topwrote:

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:

#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 rename component.json to bower.json #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

Patch Links:

Reply to this email directly or view it on GitHubhttps://github.com//pull/116
.

@majorcode
Copy link
Author

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

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

Successfully merging this pull request may close these issues.

3 participants