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

Target taps #22

Closed
wants to merge 5 commits into from
Closed

Target taps #22

wants to merge 5 commits into from

Conversation

olferuk
Copy link

@olferuk olferuk commented Jul 17, 2016

I added .targetTap and .targetLongPress events. This events supplement .tap and .longPress events, but also they hold location in view where the event was fired.

So, .targetTap works as follows:

hoursContainer.rx_gesture(.TargetTap(.Anywhere))
    .subscribeNext { gesture in
        switch gesture {
        case .TargetTap(let data):
            print(data.location) // prints location of the tap in hoursContainer view
            break
        default: break
        }
    }
    .addDisposableTo(bag)

As for .targetLongPress, there are .Any, .Began, .Changed, .Ended states as they are in Pan or Rotate events.

hoursContainer.rx_gesture(.TargetLongPress(.Began), .TargetLongPress(.Ended))
    .subscribeNext { (gesture) in
        switch gesture {
        case .TargetLongPress(let data):
            print(data.location) // prints coordinates of location where user started to hold his finger and finally released
            break
        default:
            break
        }
    }
    .addDisposableTo(bag)

I think this will be useful when user creates a custom view with specific logic.

@icanzilb
Copy link
Member

Code looks good at first sight, I'm only not completely sure about the naming ... If I see the code out of the context of this PR I would not be sure what a TargetLongPress is. Have you considered any alternatives?

@icanzilb
Copy link
Member

Okay, now I'm confused what those two recognizers are supposed to do. Could you please explain what would the two code samples included in the PR description do?

@olferuk
Copy link
Author

olferuk commented Jul 24, 2016

@icanzilb Ok, sure. I renamed Target- to Located-, because it is all about where the gesture was performed in the view's local coordinate system.
And take a look at README, I added a few more comments about how to comprehend callback data.

@icanzilb
Copy link
Member

😀 ok, so the two gestures are all about the location of the tap/press? And the only piece of data the gesture provides is the tap location?

Then what do you think of calling the gesture TapLocation and LongPressLocation? I think that would be clear to everyone then.

What do you think?

@olferuk
Copy link
Author

olferuk commented Jul 24, 2016

Yes, all about location, yes, the only data, this PR is not an overwhelming improvement. :)

Well, maybe the LocatedTap is not the best name, but it says that it is a tap. And LocatedLongPress says it is a long press gesture. TapLocation looks like it is location actually, when really it is a gesture, so this can be misleading, I suppose.

@icanzilb
Copy link
Member

I'm not saying the contribution is not significant, I'm just trying to make sure I understand what it does. I think it should be okay to merge - I'm still hesitant about the naming but you're right that otherwise is also not very clear.

@olferuk
Copy link
Author

olferuk commented Jul 24, 2016

After a while I still haven't figured out the best name for this 😃

@gkuhlmann14
Copy link

Hey guys, what's going on with this? Working on a Rx Camera app where this could be really helpful for tap to focus.

@icanzilb
Copy link
Member

This PR has conflicts - it can't be merged at this time

@jegnux jegnux mentioned this pull request Feb 12, 2017
6 tasks
@jegnux
Copy link
Member

jegnux commented Feb 20, 2017

The new 1.0.0 release introduces a new asLocation(in:) operator that suits your needs. Check it out !

@jegnux jegnux closed this Feb 20, 2017
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

Successfully merging this pull request may close these issues.

4 participants