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

[RTM] Removed dependency on setScrollableView**() #636

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[RTM] Removed dependency on setScrollableView**() #636

wants to merge 2 commits into from

Conversation

renaudcerrato
Copy link

setScrollableView(View) and its matching xml attribute umanoScrollableView may easily be replaced by simply looping over the slidable view's childrens. This allow for a cleaner API, for example when working with scrollable fragments as slidable view.

@tokudu
Copy link
Contributor

tokudu commented Jan 6, 2016

This is a bad idea. You are effectively going through the entire view hierarchy. The performance would be pretty bad for large list views.

@renaudcerrato
Copy link
Author

In 99% of cases, the scrollable view will be at the root. The main advantage is that the slidable content shouldn't have to be static anymore: the current implementation can't easily handle dynamic fragments as slidable contents... Using that simple trick, you can effectively swipe your content dynamically.

BTW, that's how Flipboard's BottomSheet is doing.

@tokudu
Copy link
Contributor

tokudu commented Jan 6, 2016

Interesting, I see your point. Though, as far as I can tell you are calling canScrollVertically recursively, so if you have a large number nested viewgroups, this would hinder the performance as well, no?

@renaudcerrato
Copy link
Author

Sure, the performance will degrade if the scrollable view is deeply nested. But as I said, in 99.9% of cases the depth won't go above 1 or 2. IMO, the benefits of having dynamic contents are worth it.

Now, I can use SlidingUpPanelLayout as a generic slidable bottom sheet, from my BottomSheetActivity.

@tokudu
Copy link
Contributor

tokudu commented Jan 6, 2016

Ok, cool. Let me digest this and play around with it when I get home tonight. Thanks for the idea and the contribution!

@renaudcerrato
Copy link
Author

You're welcome! I'll stick with my fork until then ;)

@renaudcerrato
Copy link
Author

As a side note: we could add a simple switch to explicitly enable (or disable) that feature in case the user knows there will be no scrollable content.

@tokudu
Copy link
Contributor

tokudu commented Feb 6, 2016

I'm still debating this. I tested it a bunch and I was really close to merging this, but then I thought about WebViews, which is where this approach doesn't work well. They could be really complex layouts with lots of children & sub children, which could lead to decreased performance. Thoughts?

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.

2 participants