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.
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
Scroll indicators update - positioning correction, support for both indicators at the same time #229
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 1 commit
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
There are no files selected for viewing
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.
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
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.
hey, I ran a test app with a scrollview on pure iOS, with showsXScrollIndicator off for both dimensions, and then on. I then used
po scrollView.subviews
in the debugger. If showsXScrollIndicator is off, the indicators are not in the view hierarchy at all, so it seems like iOS does not position them in that case. I could imagine that iOS might save positions for indicators without adding them to view hierarchy just yet, so maybe that's what happens - but no idea how to test that.In light of this, what do we want to do?
From the perspective of code changes, laying them out in all cases should be very easy and help us get rid of a few lines of code. So I absolutely don't mind doing it if you feel that's the way to go.
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.
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)
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.
yes, I can get the reference and print positions, even if I make them invisible with
isHidden
.but if I set
showsVerticalScrollIndicator
forfalse
, that indicator will no longer be in the view hierarchy and therefore probably impossible to get a reference to.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?
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.
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.
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.
update: merged #284