Open janek opened 6 years ago
@ephemer @janekszynal is there anything preventing us from merging this? do we need any tests?
hey @ephemer, I can't reply directly to your last comment, not sure why. It might be because I changed the code it was referencing (and it got auto-marked as outdated) or because it was a comment to the PR in general and not a line in code (and therefore doesn't have a reply thread).
Anyway, I changed the add/insert subview code. Couldn't avoid writing a few extra lines, but I think it's all pretty understandable and therefore no big harm done. Let me know what you think.
Hi @janek, I'd really like to get this PR merged because it's been open for a long time. My main concern is still this ordering of the indicators. I have the feeling that part could be buggy still. Let's take some time when you're back in the office to go through this and get it merged
yeah, I would love that too, it's been very long. Let's try tomorrow!
Merging #229 into master will increase coverage by
0.71%
. The diff coverage is97.59%
.
@@ Coverage Diff @@
## master #229 +/- ##
==========================================
+ Coverage 52.15% 52.86% +0.71%
==========================================
Files 87 87
Lines 3160 3210 +50
==========================================
+ Hits 1648 1697 +49
- Misses 1512 1513 +1
Impacted Files | Coverage Δ | |
---|---|---|
Sources/UIScrollView.swift | 90.90% <92.85%> (+2.02%) |
:arrow_up: |
Sources/UIScrollView+indicatorsInternal.swift | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 8f10d29...6a899ae. Read the comment docs.
hey @ephemer :) I improved a lot here, addressing all the smaller comments you had.
Unfortunately, I still have a concern:
if you check scrollView.subviews
on iOS, it will not include scroll indicators. In our version, in the state from this PR, it will include them. We can address this here or with a new issue, depending on how demanding/important it is. I gave it a moment's thought and decided it's best to ask you for opinion before trying, and a tip on how to go about it.
@janek we could override var subviews
to return a filtered version of private var _subviews = [UIView]
. It's tricky though because AFAIK we're using subviews
for some "internal" UIKit logic, which may or may not be correct.
But to me that's an issue that's totally separate from this PR. If you would like to update that let's do it for a practical (rather than idealistic) reason: right now I don't think it matters for us
cool, that's what I was thinking, too, for the implementation. Glad I had the right idea (and the right idea to ask).
In that case, this is ready for re-review.
thanks @ephemer! I didn't reply to all of your comments, but I took them into account when rewriting tests. It's definitely a big improvement, and I'm quite happy. Also very happy to perfect them if there is anything else that you see could be improved :)
Corrects positioning, adds configurable indicator insets, adds conditional positioning when one/both indicators are visible.
Type of change: Bug fix + feature
Todos
scrollIndicatorInsets
(as per apple docs)Expected behavior
Scroll indicators should now position and style themselves in a way indistinguishable from Apple's.
Testing Details
Tested visually on NativePlayer with some additional marker views for better confidence. Tested with a smaller dedicated app, both visually and numerically.
Screenshots
Current (desired) positioning of indicators:
Please check if the PR fulfills these requirements