-
Notifications
You must be signed in to change notification settings - Fork 25.1k
[Android] Implement ScrollView sticky headers on Android #9456
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
Closed
janicduplessis
wants to merge
10
commits into
facebook:master
from
janicduplessis:sticky-headers-android
Closed
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
5d03f76
[Android] Implement ScrollView sticky headers on Android
janicduplessis b7bf660
Remove HitTestView, make TouchTargetHelper respect drawing order
janicduplessis c76ba7a
Remove collapsableChildren
janicduplessis 70606f4
Use view matrix in view clipping
janicduplessis 0e3481c
Add DrawingOrderViewGroup interface to avoid circular dep, change bou…
janicduplessis 0cbaf25
Update view clipping when updating transform
janicduplessis 8bd3676
Revert "Remove collapsableChildren"
janicduplessis 4b6bbcd
Rename collapsableChildren to collapseChildren
janicduplessis 95b5855
Fix imports
janicduplessis 51e0acc
Fix view optimisation if parent is null
janicduplessis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
22 changes: 22 additions & 0 deletions
22
ReactAndroid/src/main/java/com/facebook/react/uimanager/DrawingOrderViewGroup.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| /** | ||
| * Copyright (c) 2015-present, Facebook, Inc. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. An additional grant | ||
| * of patent rights can be found in the PATENTS file in the same directory. | ||
| */ | ||
|
|
||
| package com.facebook.react.uimanager; | ||
|
|
||
| public interface DrawingOrderViewGroup { | ||
| /** | ||
| * Returns if the ViewGroup implements custom drawing order. | ||
| */ | ||
| boolean isDrawingOrderEnabled(); | ||
|
|
||
| /** | ||
| * Returns which child to draw for the specified index. | ||
| */ | ||
| int getDrawingOrder(int i); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I was going to comment on this, If I don't do this native view counts doesn't match sticky indices. I think it is because of
nullreact elements. Any other idea other than doing this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we're setting collapsable=false on all those children though -- I don't understand why that wouldn't be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some children are null so we can't set collapsable = false on them. I had to add a null check in children map here 1e87a60#diff-f8f8b220fc0d1573e4f8b78c84415075R455
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Then we should either have JS send the tags of the children that are sticky headers which the native view could then convert into indices (which would also fix the whole collapsable problem), or less ideally remap the indices in JS.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you go about getting the sticky headers view tags in JS? Seems like it would be relatively complex since we can only get them after the view has been rendered using refs. Maybe remapping indexes to offset for null views that will be removed would end up a lot simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok this is actually harder than I thought because just adding
collapsable=falsedoesn't really work because children are not actuallyViewcomponents so adding thecollapsableprop doesn't do anything. In the case ofListViewit's always aStaticRendererwhich ignores thecollapsableprop. I'm kind of leaning back towards adding thecollapsableChildrenprop toViewthat ensures that all it's children map to a nativeViewand don't get optimized out.Here's a simple example to show why adding the
collapsableprops toScrollViewchildren in JS doesn't work:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I understand the problems you're having. Ideally we'd have some way to refer to the child views other than by index (since it isn't stable), but that doesn't exist right now and I don't know a good way to do it. I'm fine doing collapsableChildren false in this case then. (Actually, another option would be a create a custom StickyHeader native view class that ScrollView knows about and can do instanceof checks in native... but that would require a change in API)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with collapsableChildren, this will allow to keep the same API and could even be useful in the future for other cases where we want an index based API like stickyHeaderIndices.