Adds a scroll offset manager class which caches and restores scroll offsets #63
Conversation
…lows for restoring scroll offsets via a protocol implementation. This ensures that scroll offsets are only remembered where users explicitly request them to be, meaning we won't interfere with any custom logic they already have to handle this. Also adds examples of this to the demo project
| scrollView.showsVerticalScrollIndicator = false | ||
| scrollView.showsHorizontalScrollIndicator = false | ||
|
|
||
| scrollable.scrollView?.setContentOffset(newOffset, animated: animated) |
There was a problem hiding this comment.
scrollView was unwrapped above
| //MARK: Scroll Offset Management | ||
| //MARK: | ||
|
|
||
| private lazy var scrollOffsetManager: ScrollOffsetManager = { |
There was a problem hiding this comment.
Up to you here, there's nothing wrong with doing it with = { }() in case you add properties in the future. But atm could simply do private let scrollOffsetManager = ScrollOffsetManager()
There was a problem hiding this comment.
Yeah let's go with that. It's memory footprint should be tiny anyways!
| } | ||
| } | ||
|
|
||
| func updateScrollPosition(cell: UITableViewCell, at indexPath: IndexPath) { |
There was a problem hiding this comment.
No harm in an animated parameter? :)
There was a problem hiding this comment.
Done... and documented!
|
|
||
| return TableSection( | ||
| rows: [ | ||
| CollectionRow(colours: [.red, .green, .blue, .brown, .cyan, .darkGray, .darkText]), |
There was a problem hiding this comment.
No issue, could use
(0 ..< 18).map { _ in
CollectionRow(colours: [.red, .green, .blue, .brown, .cyan, .darkGray, .darkText])
}
There was a problem hiding this comment.
Will do that! :D
| var scrollDelegate: ScrollOffsetDelegate? { get set } | ||
|
|
||
| /// An identifier used by `ScrollOffsetManager` | ||
| var identifier: AnyHashable? { get set } |
There was a problem hiding this comment.
I can defo see why if the implementor providing an identifier is real nice. Hence this. But it's a little annoying to have to provide it. Maybe I'm overlooking here, how's this going to work if some provide it and some don't?
Anyway we probably want some default implementations here? :) Like this and delegate as nil?
There was a problem hiding this comment.
So TableViewController actually sets this to the given IndexPath so all implementers have to do is provide the property. Not sure if that makes sense to you? It just means all information is stored on the cell itself, and we don't need to fetch different bits of info from different places. Maybe I need to make that clearer in the docs?
There was a problem hiding this comment.
Well Apple choose not to put an IndexPath property on the UITableViewCell. That's where the manager would hold onto that mapping. I'll come back to
There was a problem hiding this comment.
This is a great PR :) Nice one Si! 😄
Approving. My only other comment would be:
Do we need the identifier on the ScrollOffsetManagable. Apple actively choose not to have an indexPath property on the TableViewCell.
So wondering if the ScrollOffsetManager should map IndexPaths to offsets, which we invalidate when the data changes (like you've done, but simplified)
Anyway, as I say, nice work!
|
That's a great shout @BenCShutt happy to revisit! Want to keep it as-is for now, as storing the identifier on the |
Description
Adds
ScrollOffsetManagerfor storing and restoring scroll offsets via a protocol implementation.This ensures that scroll offsets are only remembered where users explicitly
request them to be, meaning we won't interfere with any custom logic they
already have to handle this.
Also adds examples of this to the demo project
Related Issue
N/A
Motivation and Context
Bugs were found in Blood within
ThunderCloudwhere scroll offsets were incorrect due to cell re-use. This allows us to handle this better by caching scroll offsets where required.How Has This Been Tested?
Has been tested using the demo project, and will shortly be tested within
ThunderCloudand specifically in the case ofBloodTypes of changes
Checklist: