Permalink
Show file tree
Hide file tree
9 comments
on commit
sign in to comment.
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Showing
2 changed files
with
21 additions
and
5 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2e10af1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats fantastic and useful addition.
2e10af1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm misunderstanding this code on first pass, but does this trigger an event that will walk up the dom everytime someone changes jQuery data? Won't this potentially cause huge performance issues?
2e10af1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would have been perfect as a plugin, considering the potential performance issues. I will no think twice before using .data()
2e10af1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the
$.data( elem, ... )
syntax instead of$(elem).data( ... )
if you don't want any of the events to fire.2e10af1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, but I still think it made more sense as a plugin.
2e10af1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justin, Irbabe: We've always fired events on the manipulation of .data() (previously just getData/setData and now with the useful addition of changeData). It looks like setData/changeData are bubbling - which should not be the case. I'll change this in a follow-up commit.
2e10af1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in c4b4df4.
2e10af1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I knew it was sending events. I was just surprised it was bubbling. I don't think data events should be bubbling by default.
However, I can definitely see a use case for data listeners - plugins that listen high on the dom for data changes and send an Ajax request back to the server -and these would require bubbling data events. But, someone should have to opt-in for this performance penalty.
BTW: this is Justin, l realized I was logged in with my other account.
2e10af1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Fixed in c4b4df4."
Yay!