Sephiroth87 / ODRefreshControl

A pull down to refresh control like the one in Apple's iOS6 Mail App
MIT License
2.14k stars 385 forks source link

Added support for scroll views with a content inset #21

Closed joein3d closed 12 years ago

joein3d commented 12 years ago

ODRefreshControl currently assumes a UIEdgeInsetsZero content inset. This doesn't work in the case where the scroll bar has a different contentInset (such as a table view displayed in a UINavigationController with a translucent navigation bar).

Sephiroth87 commented 12 years ago

Hi, thank you for pointing this out, but I'm sorry I can't merge your code, as it's not really working as it should. Right now, the control will cover the area defined by the inset, while it should go above that. Also, after triggering it, it disappears.

I'll fix it as soon as possible...

joein3d commented 12 years ago

Added a fix for the disappearance after triggering.

At least in the case I'm using (translucent nav bar), I want the control in the inset area. I added the alpha change so it wouldn't be visible behind the nav bar. (video: http://cl.ly/1G1Q3e2r172j) Are you saying the user should have to drag the control from above the screen, through the inset area, and into the main area to refresh?

Sephiroth87 commented 12 years ago

I think putting the control there is wrong, as something else can already be in the inset, and you'll be covering it. If you use the control-that-should-not-be-named-cause-nda, you'll notice it'll work this way.

It'll do the same thing also in the case of a translucent navigation bar, and it'll make the control appear behind the bar, and it'll need to be dragged the usual amount to be triggered...

joein3d commented 12 years ago

I set up the control that shall not be named with a translucent nav bar and you're right, it operates above the view then underneath the bar. Seems like a bug to me, but I guess one man's bug is another man's feature.

Sephiroth87 commented 12 years ago

I think the idea is that the control should not cover the inset area, because something could be there... The case when there's a translucent bar is probably an edge case, that looks not so good, but has the correct behaviour.

ghost commented 12 years ago

@joein3d your commit prevents the activity indicator from being show when refreshing manually if I have a content inset set.

joein3d commented 12 years ago

@CodinGuru should be fixed in the most recent commit.

ghost commented 12 years ago

@joein3d still doesn't seem to show the activity indicator when triggering beginRefreshing manually with a content inset set.

joein3d commented 12 years ago

@CodinGuru could you take a video of what you're seeing?

The bug I fixed was when you manually begin refreshing, it wouldn't set the new content inset correctly so you couldn't scroll to where the activity indicator was visible.

Is the bug you're seeing that the inset is correct but the activity indicator is invisible?

Do you expect the scroll view to scroll such that the activity indicator is visible when you begin refreshing manually? The original control doesn't do this. I could make this happen, but I didn't to keep in line with the original control. It's such that if you manually begin refreshing, the user needs to scroll up to make the activity indicator visible.

ghost commented 12 years ago

@joein3d I expected the "scroll view to scroll such that the activity indicator is visible when I begin refreshing manually." I could have sworn I saw the original control do this. It does show the activity indicator fine now when pulling down now.

cameroncooke commented 11 years ago

Can this be reopened as the current implementation makes no sense.

A normal everyday use case for using UIScrollView's content and scroller insets is to allow space for a UINavigationBar or UIToolbar that is semi-transparent. A good example of this is a fullscreen view like the UIImagePickerController.

The current implementation would put the pull-down graphic behind the navigation bar and inside the content inset area.

I personally think it should be be below the content inset as the whole point of the content inset is that it acts as a margin as in the version of the code by joein3d or at least expose an option in the API to do this.