Minor Tweaks to the Class... #2

Closed
wants to merge 4 commits into
from

2 participants

@masterexploder

First, awesome work with the library, it's been an awesome addition to the project I'm working on.

That being said, I did make a few tweaks that I wanted to share with you. Basically, these are the result of some needs that arose whilst using your class in my app... I have a lot of content that's dynamically updated via XHR calls, and I found that things would get sluggish after a while or I would end up with multiple tip instances attached to the same element. I wanted to re-use instances of the FloatingTips class, so I added a bit of functionality to make all that work.

Should be pretty self-explanatory from the commits. Feel free to pull in the changes, and no worries if you're not interested :) Either way, awesome work, thanks for sharing it with the world!

masterexploder added some commits Jul 1, 2011
@masterexploder masterexploder Prevent events from being attached more than once
We don't want to attach events to elements that already have them (i.e. somebody creates two instances of FloatingTips, perhaps the second after a content refresh via XHR), so we set a flag on the element via Element.Storage to denote the event attachment
31944ff
@masterexploder masterexploder Added ability to detach floating tip events
Since somebody might want to detach the events at some point for various reasons, tweaked the class a little bit to add a "detach" function.

Example:
var myTips = new FloatingTips('a.tips');
// down the road somewhere
myTips.detach();
// and maybe we want to re-enable them again...
myTips.attach();
ec17965
@masterexploder masterexploder Using hard tabs to match style of original code eaa54b3
@lorenzos
Owner

Hi. Thank you for your collaboration.
In fact, using AJAX loaded content causes some problems with the tips. I encountered these problems also on a project of mine.
I will pull in it for sure!

PS: I was scared seeing about 70 diffs on the main two commits, then I saw lots of them are only white space alteration :)

@lorenzos
Owner

Two issues:

  • You removed elements argument from attach method because you moved the elements list in the element class field (correctly you need it for detach). I prefer to keep it and merge passed elements in the class variable, so you can add target elements by using attach more times:
var myFloatingTips = new FloatingTips();
myFloatingTips.attach('a.tip');
myFloatingTips.attach('#content a[target=_blank]');
  • You changed show and hide methods to get target element from the passed event (e.target). The show(Element) method is useful to manually trigger a tooltip on a specified element. We can keep this behaviour by checking type of the passed argument. What do you think?
@masterexploder

To your first point: We could do this, but it should track those as members of an array... that way you can attach and detach specific groups. Also, if you passed nothing to detach, it would detach from all known elements, which would give the most flexibility (in my opinion).

The 2nd item seems pretty fair to me, I can whip something up real quick and send it over to you :)

@lorenzos
Owner

I thought about something like:

attach: function(elements) {
    if (elements) this.elements.include($$(elements));
}

This should work. include will append non-existing array items to this.elements, and $$ does the rest: if elements is an array, an array is returned, if elements is a string, then the selector engine will return an element array.

I think the detach method does not need arguments. We detach tips from all elements, just it.

@lorenzos lorenzos closed this Jul 5, 2011
@lorenzos lorenzos reopened this Jul 5, 2011
@masterexploder masterexploder Implementing suggested changes in pull request
Attach / detach now take arguments to allow for arbitrary addition or removal of tips
Show / hide functions can now take either an event argument (default), or an actual element to manually show / hide tips for said element
9f26728
@masterexploder

Any thoughts, feedback, progress with this one?

@lorenzos
Owner

As soon as I get some time, I will pull your contributes. Sorry about that.

@lorenzos
Owner

I'm experiencing problems with "inside" tip position using your source.
I will correct them ("inside" position is a little tricky) and finally merge that pull :)

@lorenzos
Owner

Ok, I fixed the "inside" problem.

There was also another tricky issue: when you check 'target' in e it return true also if e is an Element and not an Event as we want. That occours when e is a <a> element, that contains itself a target attribute! :)
So I have to double check using ('target' in e) && (typeof e.target != 'string').

Because I just merged another pull request, now I have to pull yours manually. I hope nothing will go wrong. Thank you!

@lorenzos lorenzos closed this Dec 7, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment