evgenyneu / Cosmos

A star rating control for iOS/tvOS written in Swift
MIT License
2.19k stars 368 forks source link

Reset the rating when not lifting finger? #89

Open theolof opened 6 years ago

theolof commented 6 years ago

Hi!

I'm using Cosmos in table cells.

Can I fix my issue somehow? The issue I'm having:

I want it to go back to "3", since I didn't lift the finger at "4".

Or, perhaps, make it so that it only gets selected on touchUpInside or something.

evgenyneu commented 6 years ago

Thanks for the proposal, @theolof. Could you tell me your motivation?

theolof commented 6 years ago

See attached recording. I haven't released the finger yet, but the rating changes. Even when I release the finger outside the stars and even that cell, the rating changes. I would like it to go back to the previous rating if I release the finger outside the star area. (In my example, I want "Stureplan 1" to stay at rating 3)

It's how a UIButton works, if I release inside the button - it registers a click. If I release outside - no click.

Is it possible with the current version?

screen recording

evgenyneu commented 6 years ago

Thanks for explaining. Did you try disabling "Cancellable content touches" for the scrollview? This is described here.

theolof commented 6 years ago

Yes, I tried that. That doesn't really change anything regarding my issue. That just stops the scrolling of the table view. If I move the finger outside the "star view", the new rating is still selected.

I guess what I want is to receive the updated rating when I release the finger, not when I start touching the stars.

Edit: Or actually, cosmosView.didFinishTouchingCosmos works as I expect, I only get that callback when I release the finger inside the stars. The issue I'm having, is that the stars get "stuck" on the wrong value, even if I release the finger outside the stars.

That means that it looks like the user has updated the rating to 4, but I haven't received a callback to didFinishTouchingCosmos since I release the finger outside the stars.

Does it make sense?

evgenyneu commented 6 years ago

Yes, I can reproduce this problem in the demo app now. It does not call didFinishTouchingCosmos in a table view if I lift the finger outside the cell. However, it started to call didFinishTouchingCosmos after I unchecked "Can cancel on scroll" checkbox for the table view. Does it help you?

table_view_can_cancel_on_scroll
theolof commented 6 years ago

Hmm, I cannot find that property in code on a tableView. I don't use Storyboard.

However, I don't think that will help me. I want the opposite. I think it's correct that it does not call didFinishTouchingCosmos when lifting the finger outside. What I need is the stars to go back to the original state if I lift the finger outside. If I lift the finger inside, didFinishTouchingCosmos should be called and the star state should reflect the position where I lift the finger.

The Google Maps app shows this behavior when creating a review. When I swipe the finger right or left, the stars updates, but if I move the finger outside the stars, the state reverts back to the original. It works like a cancel action.

evgenyneu commented 6 years ago

I think I found a fix for this. When Cosmos view is in a scrollview cell its touches are cancelled if one moves finger out of the cell. This is what happens in our case. All I need to do is to call didFinishTouchingCosmos when touches are cancelled. This will makes sure there is no disconnect between the displayed and reported rating. Will it solve your problem?

theolof commented 6 years ago

Sorry, I don't think so. The user will expect the rating to be cancelled when moving outside the stars area. Your change will update the rating, but the user would expect it to be cancelled.

On the other hand, it's better than now, since it will make the displayed and reported rating the same at least.

evgenyneu commented 6 years ago

Hmm, I think you are right, however if I revert to the old rating on touchesCancelled event it looks like there a bug with cosmos view. For example, if I want to swipe all the way to the right to give it 5 stars it sometimes just reverts to zero. Doesn't feel good. See the animation below.

cancel_touches

theolof commented 6 years ago

Yes...I see the problem.

It would be nice to have "revert if dragging down or up" but "set the rating if dragging left or right". Don't know if that's possible.

It looks like that's how the google maps app is handling it.

theolof commented 6 years ago

UX is tricky...Maybe it needs to check how far outside the user has dragged the finger, and only revert if the distance is more than X.

evgenyneu commented 6 years ago

Maybe it needs to check how far outside the user has dragged the finger, and only revert if the distance is more than X.

I just tried this, increased the hit area that is allowed to lift the finger. It fills broken to me as a user if I swipe all the way and see it revert the rating. I don't know...doing additional check about the swipe direction can complicate the code and lead to surprises. I understand what you are saying about the default functionality of a button and user expectations though...

I will call didFinishTouchingCosmos on touchesCancelled for now as a quick fix and will think how to deal with rating reversal later, ok?

theolof commented 6 years ago

Sure, that will keep them in sync.

evgenyneu commented 6 years ago

Released the fix in version 13 to Cocoapods. Thanks for reporting the issue! 👍

evgenyneu commented 6 years ago

The rating in the App Store app does not revert to old rating if finger is lifted far away from the view. It works exactly like Cosmos at the moment. Not sure if it's what user expects though.

theolof commented 6 years ago

You're right! I think Apple is wrong on that one, there's no way to cancel the rating... But if Apple does it, maybe you can leave it as it is...

theolof commented 6 years ago

I've tested it UX-wise a while now, and I accidentally create ratings too often when all I want is to scroll the table.

A workaround for me is to disable the "swipe" and only trigger on touchesEnded.

I override CosmosView and do this:

    open override func touchesBegan(_ touches: Set<UITouch>, with event: UIEvent?) {
        return
    }

    open override func touchesMoved(_ touches: Set<UITouch>, with event: UIEvent?) {
        return
    }

    open override func touchesCancelled(_ touches: Set<UITouch>, with event: UIEvent?) {
        return
    }

    open override func touchesEnded(_ touches: Set<UITouch>, with event: UIEvent?) {
        super.touchesMoved(touches, with: event)
    }

It's not a good solution code-wise, and there are possibly side-effects when I do this.

Any suggestions?

evgenyneu commented 6 years ago

If it works for you then it is fine. Alternatively, one can use the scroll view only for displaying ratings and create a separate view for changing it (in a modal popup or a separate view controller).

dabinder93 commented 6 years ago

@theolof hey :) I know it has been a long time since your problem occurred, but I have the same problem now. I got a table view with different cells in it, one of the cells contains the star rating, others textviews etc. when I scroll through the table, I often accidentally hit the stars. in my app, the rating value gets fetched from the cosmos view, when I press on the cell (so on the willSelectRowAt delegate). When I scroll through the list and accidentally hit the stars, they get selected but the willSelectRowAt doesn't trigger (only on tap, obviously). Now I would like to disable the swipe function like you did, but my cell already inherits from UITableViewCell, so I can't override the cosmosView methods.

any suggestions?

with friendly regards, David

theolof commented 6 years ago

Sorry, I ended up removing the stars on the cells, it didn't work well enough. I had to use the stars on another view, after tap on the cell.