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

{{data-attribute}} Not Matching #2

Open
tpitale opened this issue Jan 12, 2012 · 10 comments
Open

{{data-attribute}} Not Matching #2

tpitale opened this issue Jan 12, 2012 · 10 comments

Comments

@tpitale
Copy link

tpitale commented Jan 12, 2012

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
Copy link
Author

tpitale commented Jan 12, 2012

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
Copy link
Author

tpitale commented Jan 12, 2012

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
Copy link
Author

tpitale commented Jan 12, 2012

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

@averyvery
Copy link
Owner

Curses. Will look at this today.

@tpitale
Copy link
Author

tpitale commented Jan 12, 2012

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

@averyvery
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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

@tpitale
Copy link
Author

tpitale commented Jan 12, 2012

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

@averyvery
Copy link
Owner

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 : [...]
      }
  }

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

No branches or pull requests

2 participants