-
Notifications
You must be signed in to change notification settings - Fork 39
Scroll indicators update - positioning correction, support for both indicators at the same time #229
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
base: master
Are you sure you want to change the base?
Scroll indicators update - positioning correction, support for both indicators at the same time #229
Changes from 66 commits
a699f9c
61cf77b
7c048e8
c1330a0
8324590
9f1538c
d4d833e
bb7abba
6ce8af6
15a32c5
ebb41cd
19611b4
5f3149e
e11665c
696b5ec
4a0d458
9df9578
4b165b7
54afeee
4b2ed66
a09c179
f26136c
9042ce9
f3acd87
59bfdfd
e5d8f8f
9a0f804
68edcf5
e03b6d0
59b1d9d
810f0ef
a8efa15
7f4d5a7
0137095
eaf1e88
2c77d5c
c43e4d9
b13e05d
3158be6
ff215e1
7086a9d
df2624b
3a24ba3
b9b6df4
5ec8cf7
975f308
e2e62d0
fd3218f
63c89f4
497b07c
d94bead
399d18e
faa63de
646b7d2
be7e9cf
8060600
7b9e543
6af7c80
19c16fd
08ff58e
f24db7e
66938cf
91ec3cd
d1a2e74
d19f0af
75553c8
147831f
df0b435
6cc4e28
bc35223
b5dcd2a
c39799b
ead7c57
1649892
2c1a237
ab05997
b22318a
7ac4e70
60d850f
65c5f0c
0e44547
fd14007
2039d6d
b2e2c46
6379d21
fa4b095
abc824c
b3e59f0
17e0f0a
658a4a2
7298827
7d39faa
8c57422
cc0452b
8a81fe4
ac1f2ec
36e63e4
6a899ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,22 +10,47 @@ | |
| // You can't override methods that were defined in an extension. | ||
|
|
||
| internal extension UIScrollView { | ||
| internal static let indicatorDistanceFromScrollViewFrame: CGFloat = 2.5 | ||
| internal var indicatorLengths: (horizontal: CGFloat, vertical: CGFloat) { | ||
| let minIndicatorLength: CGFloat = 30.0 | ||
| return ( | ||
| horizontal: max(minIndicatorLength, (bounds.width / contentSize.width) * bounds.width), | ||
| vertical: max(minIndicatorLength, (bounds.height / contentSize.height) * bounds.height) | ||
| ) | ||
| } | ||
|
|
||
| internal var indicatorOffsetsInContentSpace: (horizontal: CGFloat, vertical: CGFloat) { | ||
| let indicatorDistanceFromScrollViewFrame: CGFloat = 2.5 | ||
|
|
||
| internal func indicatorOffsetsInContentSpace() -> (horizontal: CGFloat, vertical: CGFloat) { | ||
| let totalContentArea = ( | ||
| horizontal: contentInset.left + contentSize.width + contentInset.right, | ||
| vertical: contentInset.top + contentSize.height + contentInset.bottom | ||
| ) | ||
|
|
||
| let scrollViewProgress = ( | ||
| horizontal: (contentInset.left + contentOffset.x) / (totalContentArea.horizontal + bounds.width), | ||
| vertical: (contentInset.top + contentOffset.y) / (totalContentArea.vertical + bounds.height) | ||
| horizontal: (contentInset.left + contentOffset.x) / (totalContentArea.horizontal - bounds.width), | ||
| vertical: (contentInset.top + contentOffset.y) / (totalContentArea.vertical - bounds.height) | ||
| ) | ||
|
|
||
| let shouldShowBothIndicators = shouldLayoutHorizontalScrollIndicator && shouldLayoutVerticalScrollIndicator | ||
|
|
||
| let additionalSpacingToPreventOverlap = ( | ||
|
janek marked this conversation as resolved.
|
||
| horizontal: shouldShowBothIndicators ? 2 * indicatorDistanceFromScrollViewFrame : 0, | ||
| vertical: shouldShowBothIndicators ? indicatorDistanceFromScrollViewFrame : 0 | ||
| ) | ||
|
|
||
| let totalSpacingFromFrameSides = ( | ||
| horizontal: totalScrollIndicatorInsets.left + totalScrollIndicatorInsets.right + additionalSpacingToPreventOverlap.horizontal, | ||
| vertical: totalScrollIndicatorInsets.bottom + totalScrollIndicatorInsets.top + additionalSpacingToPreventOverlap.vertical | ||
| ) | ||
|
|
||
| let lengthOfAvailableSpaceForIndicators = ( | ||
| horizontal: bounds.width - (indicatorLengths.horizontal + totalSpacingFromFrameSides.horizontal), | ||
| vertical: bounds.height - (indicatorLengths.vertical + totalSpacingFromFrameSides.vertical) | ||
| ) | ||
|
|
||
| let indicatorOffsetInBounds = ( | ||
| horizontal: scrollViewProgress.horizontal * bounds.size.width, | ||
| vertical: scrollViewProgress.vertical * bounds.size.height | ||
| horizontal: scrollViewProgress.horizontal * lengthOfAvailableSpaceForIndicators.horizontal, | ||
| vertical: scrollViewProgress.vertical * lengthOfAvailableSpaceForIndicators.vertical | ||
| ) | ||
|
|
||
| return ( | ||
|
|
@@ -42,28 +67,27 @@ internal extension UIScrollView { | |
| return showsVerticalScrollIndicator && contentSize.height > bounds.height | ||
| } | ||
|
|
||
|
|
||
| internal func layoutScrollIndicatorsIfNeeded() { | ||
| guard shouldLayoutHorizontalScrollIndicator || shouldLayoutVerticalScrollIndicator else { return } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. iOS always lays out the scroll indicators I think, and there was an advantage to this -> I seem to remember finding a bug where the indicators appeared in the wrong spot after being hidden. I think we need to revisit this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hey, I ran a test app with a scrollview on pure iOS, with showsXScrollIndicator off for both dimensions, and then on. I then used In light of this, what do we want to do?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi Janek, you can get a reference to the scroll indicator subviews (just iterate through them on an empty scroll view and find the UIImageViews) and then hide them. You should be able to print their positions at every pixel scrolled. Would be cool if you could check this. I think it makes sense to remove them from the hierarchy of they’re hidden, but laying them out shouldn’t be a big performance issue (if that’s what iOS does)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I can get the reference and print positions, even if I make them invisible with but if I set I checked back with our code and this is where we differ - we always have indicators in the hierarchy and just make them invisible if the UIKit user wants to hide them. So if they are set to hidden by UIKit API, we keep them in the hierarchy, but don't update their positions, and then start updating it when they become visible. We could:
v1. and v2. are equally easy. We are in state 1 and if we want 2, we should just merge this new small PR I opened (#284). It's set to merge into this branch, not master. v3. is more work and I worry it might also complicate things with our inserting/removing subviews code, so I'd rather not do it now - but again, it seems the closest to iOS. Maybe let's go with 1 or 2 and create a low-prio issue for 3?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update: I now changed the insert/add subview code - didn't take long at all and should now be robust w.r.t indicators sometimes not being there. It therefore also shouldn't be too much pain to implement v3. I'll still wait for your opinion before starting on that.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update: merged #284 |
||
|
|
||
| let indicatorDistanceFromScrollViewFrame = UIScrollView.indicatorDistanceFromScrollViewFrame | ||
| let indicatorOffsets = indicatorOffsetsInContentSpace() | ||
| let indicatorLengths = (horizontal: (bounds.width / contentSize.width) * bounds.width, | ||
| vertical: (bounds.height / contentSize.height) * bounds.height) | ||
| let distanceFromFrame = ( | ||
| horizontal: indicatorThickness + totalScrollIndicatorInsets.bottom, | ||
| vertical: indicatorThickness + totalScrollIndicatorInsets.right | ||
| ) | ||
|
|
||
| if shouldLayoutHorizontalScrollIndicator { | ||
| horizontalScrollIndicator.frame = CGRect( | ||
| x: indicatorDistanceFromScrollViewFrame + indicatorOffsets.horizontal, | ||
| y: bounds.height - (indicatorThickness + indicatorDistanceFromScrollViewFrame), | ||
| x: totalScrollIndicatorInsets.left + indicatorOffsetsInContentSpace.horizontal, | ||
| y: bounds.height + contentOffset.y - distanceFromFrame.horizontal, | ||
| width: indicatorLengths.horizontal, | ||
| height: indicatorThickness | ||
| ) | ||
| } | ||
|
|
||
| if shouldLayoutVerticalScrollIndicator { | ||
| verticalScrollIndicator.frame = CGRect( | ||
| x: bounds.width - (indicatorThickness + indicatorDistanceFromScrollViewFrame), | ||
| y: indicatorDistanceFromScrollViewFrame + indicatorOffsets.vertical, | ||
| x: bounds.width + contentOffset.x - distanceFromFrame.vertical, | ||
| y: totalScrollIndicatorInsets.top + indicatorOffsetsInContentSpace.vertical, | ||
| width: indicatorThickness, | ||
| height: indicatorLengths.vertical | ||
| ) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.