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.

Sign up for GitHub

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

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

Scroll indicators update - positioning correction, support for both indicators at the same time #229

Changes from 1 commit
Commits
Show all changes
98 commits
Select commit Hold shift + click to select a range
a699f9c
corrected positioning code
janek Jun 7, 2018
61cf77b
added a (hopefully temporary) decay term to offset calculation
janek Jun 7, 2018
7c048e8
included additional insets in the positioning, eliminated decayTerm
janek Jun 13, 2018
c1330a0
ensured default insets load appropriately, added small fixes
janek Jun 13, 2018
8324590
lay out indicators out to their initial position when new insets are …
janek Jun 13, 2018
9f1538c
Corrected indicators positioning
janek Jun 29, 2018
d4d833e
Added initial distance from scrollview frame
janek Jun 29, 2018
bb7abba
support for both indicators at once
janek Jun 29, 2018
6ce8af6
additional spacing that prevents overlap when both indicators are pre…
janek Jun 29, 2018
15a32c5
connected confiugrable insets code to placement code
janek Jun 29, 2018
ebb41cd
abstracted out 1 additional step for clarity, corrected var privacy
janek Jun 30, 2018
19611b4
correction for overlap-avoidance, style improvements
janek Jul 1, 2018
5f3149e
corrected syntax bug
janek Jul 5, 2018
e11665c
corrected positioning code
janek Jun 7, 2018
696b5ec
added a (hopefully temporary) decay term to offset calculation
janek Jun 7, 2018
4a0d458
included additional insets in the positioning, eliminated decayTerm
janek Jun 13, 2018
9df9578
ensured default insets load appropriately, added small fixes
janek Jun 13, 2018
4b165b7
lay out indicators out to their initial position when new insets are …
janek Jun 13, 2018
54afeee
Corrected indicators positioning
janek Jun 29, 2018
4b2ed66
Added initial distance from scrollview frame
janek Jun 29, 2018
a09c179
support for both indicators at once
janek Jun 29, 2018
f26136c
additional spacing that prevents overlap when both indicators are pre…
janek Jun 29, 2018
9042ce9
connected confiugrable insets code to placement code
janek Jun 29, 2018
f3acd87
abstracted out 1 additional step for clarity, corrected var privacy
janek Jun 30, 2018
59bfdfd
correction for overlap-avoidance, style improvements
janek Jul 1, 2018
e5d8f8f
corrected syntax bug
janek Jul 5, 2018
9a0f804
Merge branch 'scroll-indicators-update' of github.com:flowkey/UIKit-S…
janek Jul 13, 2018
68edcf5
ensured scroll indicators stay on top of all other subviews of UIScro…
janek Jul 13, 2018
e03b6d0
Merge branch 'master' into scroll-indicators-update
ephemer Jul 17, 2018
59b1d9d
Implement some PR feedback
ephemer Jul 17, 2018
810f0ef
Merge branch 'scroll-indicators-update' of github.com:flowkey/UIKit-S…
janek Jul 23, 2018
a8efa15
more PR feedback changes
janek Jul 25, 2018
7f4d5a7
corrected UIEdgeInsets code
janek Jul 25, 2018
0137095
corrected positioning code
janek Jun 7, 2018
eaf1e88
added a (hopefully temporary) decay term to offset calculation
janek Jun 7, 2018
2c77d5c
included additional insets in the positioning, eliminated decayTerm
janek Jun 13, 2018
c43e4d9
ensured default insets load appropriately, added small fixes
janek Jun 13, 2018
b13e05d
lay out indicators out to their initial position when new insets are …
janek Jun 13, 2018
3158be6
Corrected indicators positioning
janek Jun 29, 2018
ff215e1
Added initial distance from scrollview frame
janek Jun 29, 2018
7086a9d
support for both indicators at once
janek Jun 29, 2018
df2624b
additional spacing that prevents overlap when both indicators are pre…
janek Jun 29, 2018
3a24ba3
connected confiugrable insets code to placement code
janek Jun 29, 2018
b9b6df4
abstracted out 1 additional step for clarity, corrected var privacy
janek Jun 30, 2018
5ec8cf7
correction for overlap-avoidance, style improvements
janek Jul 1, 2018
975f308
corrected syntax bug
janek Jul 5, 2018
e2e62d0
corrected positioning code
janek Jun 7, 2018
fd3218f
added a (hopefully temporary) decay term to offset calculation
janek Jun 7, 2018
63c89f4
included additional insets in the positioning, eliminated decayTerm
janek Jun 13, 2018
497b07c
ensured default insets load appropriately, added small fixes
janek Jun 13, 2018
d94bead
lay out indicators out to their initial position when new insets are …
janek Jun 13, 2018
399d18e
Corrected indicators positioning
janek Jun 29, 2018
faa63de
Added initial distance from scrollview frame
janek Jun 29, 2018
646b7d2
support for both indicators at once
janek Jun 29, 2018
be7e9cf
additional spacing that prevents overlap when both indicators are pre…
janek Jun 29, 2018
8060600
connected confiugrable insets code to placement code
janek Jun 29, 2018
7b9e543
abstracted out 1 additional step for clarity, corrected var privacy
janek Jun 30, 2018
6af7c80
correction for overlap-avoidance, style improvements
janek Jul 1, 2018
19c16fd
corrected syntax bug
janek Jul 5, 2018
08ff58e
ensured scroll indicators stay on top of all other subviews of UIScro…
janek Jul 13, 2018
f24db7e
Implement some PR feedback
ephemer Jul 17, 2018
66938cf
more PR feedback changes
janek Jul 25, 2018
91ec3cd
corrected UIEdgeInsets code
janek Jul 25, 2018
d1a2e74
Merge branch 'scroll-indicators-update' of https://github.com/flowkey…
janek Jul 25, 2018
d19f0af
rename 'bothIndicatorsShowing'
janek Jul 25, 2018
75553c8
removed whitespace
janek Jul 25, 2018
147831f
Merge branch 'master' into scroll-indicators-update
rikner Mar 4, 2019
df0b435
changed internal access levels to private in indicators extension, re…
janek Mar 5, 2019
6cc4e28
updated UIScrollViewIndicatorStyle to be UIScrollView.IndicatorStyle …
janek Mar 5, 2019
bc35223
Added comments explaining source of thickness and inset values
janek Mar 5, 2019
b5dcd2a
brought back explicit 'internal' access level keywords
janek Mar 11, 2019
c39799b
improved safety of code responsible for adding subviews: we now check…
janek Mar 11, 2019
ead7c57
Test push
janek Apr 30, 2019
1649892
removed conditions for laying out indicators, it will now happen ever…
janek Mar 11, 2019
2c1a237
Removing testfile
janek Apr 30, 2019
ab05997
Merge branch 'master' into scroll-indicators-update
janek Feb 12, 2020
b22318a
Linting: spaces should be on both sides of the minus sign
janek Feb 12, 2020
7ac4e70
Added a comment about distance values mimicking iOS
janek Feb 12, 2020
60d850f
Temporary changes to the Demo App
janek Feb 17, 2020
65c5f0c
Add a test for scroll veiw with scroll indicators subview hierarchy
janek Feb 17, 2020
0e44547
Added information about indicators being accesible in `subviews` and …
janek Feb 17, 2020
fd14007
Fix mockTouch
janek Feb 17, 2020
2039d6d
Add a guard to 'layout indicators if necessary'
janek Feb 17, 2020
b2e2c46
FIx whitespace
janek Feb 17, 2020
6379d21
simplified `earliestIndicatorInSubviewHierarchy`
janek Feb 17, 2020
fa4b095
Improve subview count test
janek Feb 17, 2020
abc824c
Remove the stub for overriding `subviews`, as default is correct
janek Feb 17, 2020
b3e59f0
Update DemoApp
janek Feb 18, 2020
17e0f0a
Improve style in addSubview
janek Feb 18, 2020
658a4a2
Add more hierarchy positions tests
janek Feb 18, 2020
7298827
Improve tests, refactor insertSubview
janek Feb 18, 2020
7d39faa
Add a comment stating that `applyScrollIndStyle` is called on init (a…
janek Feb 18, 2020
8c57422
Update the DemoApp to change black to blue
janek Feb 18, 2020
cc0452b
Restore ViewController for master
janek Feb 18, 2020
8a81fe4
Divide test into two, clarify purpose and remove bug
janek Feb 18, 2020
ac1f2ec
Improve comment and test names
janek Feb 18, 2020
36e63e4
Merge commit '8f10d29ea93f93ca65633c6bd1f1b863d2461b5a' into scroll-i…
janek Feb 19, 2020
6a899ae
Update insertSubview test, fix a bug in insertSubview
janek Feb 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
removed whitespace
  • Loading branch information
@janek
janek committed Jul 25, 2018
commit 75553c8e4e67939cc2c88e634010e930dae82a84
2 changes: 1 addition & 1 deletion Sources/UIScrollView+indicatorsInternal.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ internal extension UIScrollView {

internal func layoutScrollIndicatorsIfNeeded() {
guard shouldLayoutHorizontalScrollIndicator || shouldLayoutVerticalScrollIndicator else { return }
Copy link
Member

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

Copy link
Contributor Author

@janek janek Mar 5, 2019 •
edited
Loading

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.

Copy link
Member

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)

Copy link
Contributor Author

@janek janek Mar 11, 2019 •
edited
Loading

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 for false, 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:

  1. keep it as it is
  2. always update their positions, even when they are invisible
  3. try to get as close to iOS as possible and actually remove them and add back as needed.

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update: merged #284


let distanceFromFrame = (
horizontal: indicatorThickness + totalScrollIndicatorInsets.bottom,
vertical: indicatorThickness + totalScrollIndicatorInsets.right
Expand Down