designatednerd / DNSSwipeableTableCell

UITableViewCell subclass to add multiple buttons to a swipe-under menu like iOS 7 Mail.
MIT License
175 stars 24 forks source link

Various improvements #3

Closed markflowers closed 10 years ago

markflowers commented 10 years ago

Major changes:

designatednerd commented 10 years ago

Hey @markflowers! Thanks for taking a look at this stuff - can you please pull from upstream and @ me once you've fixed the merge conflicts?

I made some changes yesterday along similar lines for the slippery panning issue, although using a different technique.

Also, on the index vs. indexPath changes - is there handling for there being more than one section? I was initially looking at that approach but it wouldn't work with sectioned tableviews.

markflowers commented 10 years ago

I can pull in the upstream, but I think the issue that you fixed yesterday should probably be revisited similarly to how I fixed the issue for the horizontal scrolling. I can look at it, and try to handle it the same way. Keeping a flag on the tableViewController just isn't a sustainable way to do it, it moves too much implementation out of the library for this.

I don't know why the indexPath shouldn't work for a grouped tableView.

designatednerd commented 10 years ago

Hm. I'll take a longer look once you've pulled in upstream. On that note, you'll see when you pull in that, I went with the number-boxing technique mentioned in this article to fix the compiler bitching about ints on 32 vs. 64 bit.

markflowers commented 10 years ago

@designatednerd I just fixed the cells partially showing issue in a more concise and isolated way by checking the initial translation of the pan to see if the user is swiping vertically or horizontally. This is how Apple handles it I'm pretty sure.

If you want to take a look at it, and if you agree that this solution is better then if you want to rollback the other commit, and then I can merge the other changes you made from yesterday and then we can move forward.

designatednerd commented 10 years ago

OK, I'll have to take a look later this evening.

designatednerd commented 10 years ago

OK, after further review, I like some of your changes but they do conflict with a bunch of others that I've made - I also noticed a mistake I made when going through your code. I'm going to pull your changes in on a separate branch and give you a heads up when they're up for you to take a look at.

markflowers commented 10 years ago

Ok, it looks like you can possibly use NSIndexPath *indexPath = [self.tableView indexPathForRowAtPoint:cell.center]; instead if the cell is returning nil for some reason. I just really don't think that you should ever store the indexPath on the cell itself, the cell shouldn't know where it is in the table, that's the reason that the delegate separates it out like that.

designatednerd commented 10 years ago

Looks like indexPathForRowAtPoint is the way to go - comparing the results from that to a stored index path they're identical.

And I 100% agree on the theory - you'll note that fixing this hack is (now was) the top of my list of TODOs, I just needed to find something that worked more consistently than indexPathForCell.

designatednerd commented 10 years ago

OK, FYI, here's how I've merged in the changes from your fork and made the updates necessary to get everything to work as it did before: https://github.com/designatednerd/DNSSwipeableTableCell/tree/flowers-integrate - Barring any objections, I'll merge that branch down and close out this PR. Sound good?

markflowers commented 10 years ago

Awesome, yeah I looked at the clean-ups that you did on the code, looks good. Thanks!

designatednerd commented 10 years ago

Merged, this is now in as the 1.0.0 release. Thanks for your help!

markflowers commented 10 years ago

No problem. Glad I could contribute!