averyvery / trackiffer

The easy GA event tracker that's hard to say.
30 stars 1 forks source link

{{data-attribute}} Not Matching #2

Open tpitale opened 12 years ago

tpitale commented 12 years ago

Because of the change from the previous issue fixing delegation, the $elem in getReplacement() is not what I would expect.

Doing this doesn't work:

trackiffer({
  'body.controller.index' : {
    delegate : '#some_form',
    rule : ['_trackEvent', 'form', 'submit', '{{data-thing-id}}']
  }
});

Doing this does:

trackiffer({
  'body.controller.index' : {
    delegate : '#some_form',
    rule : ['_trackEvent', 'form', 'submit', function($elem){return $elem.find("#some_form").attr('data-thing-id')}]
  }
});

$elem appears to be assigned to the matched element for 'body.controller.index', rather than the delegated match.

tpitale commented 12 years ago

As another example, this in the README:

trackiffer({
  'body' : {
      delay : false,
      delegate : 'a',
      rule : ['_trackEvent', 'Link', 'Social', '{{text}}', 1]
   }
});

Would return all the text in the body.

tpitale commented 12 years ago

This line:

var formatted_event_data = _t.formatData(event_data.rule.slice(0), $elem);

Could be changed to something like this:

var formatted_event_data = _t.formatData(event_data.rule.slice(0), jQuery(this));
tpitale commented 12 years ago

However, there may be greater repercussions in that handler function.

averyvery commented 12 years ago

Curses. Will look at this today.

tpitale commented 12 years ago

I tested my suggested change, it works for me. Do you want a pull request?

averyvery commented 12 years ago

Feel free to keep using your fixed version, but you were right, this goes a little deeper. Will push a broader fix this evening.

averyvery commented 12 years ago

Okay, should be fixed.

There's another bug, at the moment, where you can't do the following:

'.config' : {
    delegate : 'a.delegated',
    rule : [...]
},
'.config' : {
    delegate : 'form',
    type : 'submit',
    rule : [...]
}

...because, of course, you're overwriting the previous .config. Will start my own issue on this, but it will require some other, less pleasant changes.

averyvery commented 12 years ago

Scratch my previous comment, that can be resolved by just calling Trackiffer a second time with the other '.config'.

tpitale commented 12 years ago

It would be cool if it could all be in one block, but if that is a nasty fix, no worries.

averyvery commented 12 years ago

Well, it would be a big syntax change if we wanted to continue using standard JS methods - it might look like...

trackiffer([
  {
      selector : 'a.config'
      rule : [...]
  },
  {
      selector : 'a.config'
      rule : [...]
  }
]);

...or maybe...

trackiffer([
  {
      'a.config' : [...]
  },
  {
      'a.config' : {
        delegate : 'span',
        rule : [...]
      }
  }
]);

...or even...

trackiffer(
  {'a.config' : [...]},
  {'a.config' : {
    delegate : 'span'
    rule : [...]
  }
)

Not thrilled with an of these. An alternative would be a little workaround, which is cute but introduces a new idea (Trackiffer-specific syntax) into a known concept (jQuery selectors):

  {
      'a.config%1' : [...],
      'a.config%2' : {
        rule : [...]
      }
  }