-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat(swipe): allow accessing the original currentTarget #10997
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
src/components/swipe/swipe.js
Outdated
@@ -85,8 +85,9 @@ function getDirective(name) { | |||
return { restrict: 'A', link: postLink }; | |||
function postLink(scope, element, attr) { | |||
var fn = $parse(attr[directiveName]); | |||
var currentTarget = ev.currentTarget; |
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.
ev
doesn't appear to be defined at this point?
Thank you very much for your contribution! In trying to better understand the requested changes, I have a couple of questions. Is there an existing issue that covers the problem being solved here in more detail? Is there a CodePen that demonstrates the issue? |
@Splaktar Thanks a lot for reviewing this! First, I fixed the scoping. I also created a code-pen to demonstrate the problem. You will see there 2 directives, both with an inner element. On the first directive, when u click the inner element, it will print on the console, the element u clicked on, and also the parent element that the |
I see. The result is a little different on AngularJS 1.6.6. I created a CodePen for that. But either way, I do see how it fails. It's unfortunate that the swipe component has no tests. It might be helpful to add a small demo of this feature to the PR. However, what is the primary use case for this feature? I.e. how would it be possible to create a helpful and clear demo. |
I needed it in a very simple use-case actually - I have a list of items (in my case they were messages). On each 'item' I added the In many cases the right way to tackle this would be to use your 'model' and not reach the DOM element directly, although I still think the framework should allow this. Where should I add a demo to the PR ? (The place I used it for isn't public, so I can't publish that). |
@Splaktar - Will this be merged? |
OK, thank you for the explanation. That makes a lot of sense and seems like an extremely common use case. Sorry for the delay. Can you please update the existing demo at https://github.com/angular/material/tree/master/src/components/swipe/demoBasicUsage to include a demonstration of this feature? |
I think that adding the events $log.log('Event Target: ', ev.target);
$log.log('Event Current Target: ', ev.currentTarget);
$log.log('Original Current Target: ', target.current); Example output from my testing: Event Target: <span class="no-select">Swipe me to the left</span>
Event Current Target: null
Original Current Target: <div class="demo-swipe" md-swipe-left="onSwipeLeft($event, $target)">…</div> The docs here should also be updated to show passing the params (i.e. |
@Splaktar - I updated the pull request with proper documentation, and logs in the demo. |
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.
Excellent, this is looking good, thank you! I just need one update for consistency.
src/components/swipe/swipe.js
Outdated
@@ -62,9 +80,15 @@ | |||
* The md-swipe-down directive allows you to specify custom behavior when an element is swiped | |||
* down. | |||
* | |||
* > You can see this in action on the <a ng-href="demo/swipe">demo page</a> (Look at the Developer Tools console while swiping). |
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.
Please make this consistent with the others, I.e. this message under the Notes.
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.
My bad. Fixed this.
Thanks a lot - This is my first contribution to a big/popular open source repo, so very excited about that! 🎉
@Splaktar Fixed the inconsistency issue. Thanks for catching that. :) |
@gillyb thank you very much! I believe that only 1 step remains 😄 Please following these steps to squash your changes into a single commit. |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
@Splaktar so.. I took your advice and changed the commit history to squash the commits into one. Doing this too fast, led to me adding my changes to the last commit on the original branch (changing the branch's history). |
Now all my changes are in a single commit, and I believe it's safe to merge. |
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.
LGTM
Thank you very much for squashing the commits. Can you please amend your commit and force push just one more time? Please update the commit to follow the commit message guidelines. This will make sure that the change gets reported in our CHANGELOG correctly when a release happens. I think that |
@Splaktar No problem, done 👍 |
When using the
md-swipe-xxxx
events, if you pass the$event
and try to access$event.currentTarget
you getnull
.I am saving a reference to the original
currentTarget
and passing it on here, via$target
object so we can use it.Why do we even need
currentTarget
?Let's say you add the
md-swipe-right
directive to a div, and that div has a few child nodes. When the user swipes, the$event.target
is the 'div' element the user swiped on, and not the 'div' element that we placed themd-swipe-right
attribute on.With other angular events (like 'on-click') we can get the div we want with
$event.currentTarget
, but this does't work for 'swipe' events.This is basically what is stated here (but not for swipe events) :
https://stackoverflow.com/questions/29421360/angular-ng-click-event-passes-child-element-as-target
and here :
https://stackoverflow.com/questions/23107613/get-original-element-from-ng-click