-
Notifications
You must be signed in to change notification settings - Fork 28
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
Support tappable headers #193
Conversation
fc59087
to
69ccbe1
Compare
We should allow pinning items, so these headers can be pinned |
@@ -10,12 +10,12 @@ import Foundation | |||
|
|||
extension ListView | |||
{ | |||
struct VisibleContent | |||
final class VisibleContent |
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.
This is needed to fix an unrelated bug that I found working on this; needs to be a class to avoid exclusive access crashes.
1e27376
to
316af9a
Compare
…-global-index * origin/main: Add demo for manual selection management
Alright, this should be good to go now |
|
||
switch state { | ||
|
||
case .possible, .began, .changed: |
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.
Is it right to include possible
here? The docs say that's the "default" state, but maybe that's not true for concrete subclasses?
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.
Ahh yeah, doh. I wonder how this works at all...
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.
Fixed!
case .ended, .cancelled, .failed: | ||
let didEnd = state == .ended | ||
|
||
UIView.animate(withDuration: didEnd ? 0.1 : 0.0) { |
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.
UITableView
doesn't animate deselection except when specified, and it's more like 0.3s - not sure we shouldn't do this, but seems rather arbitrary
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.
I wanted this animated, since it's basically like a button press, and it looks nicer when the press ends with an animation, IMO.
} | ||
|
||
|
||
fileprivate final class PressGestureRecognizer : UILongPressGestureRecognizer { |
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.
I'm curious why you're using a long press gesture for this? Does it recognize a quick tap?
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.
I needed a continuous gesture recognizer, a tap recognizer is discrete (it calls you on tap and then is done), which isnt how row selection works in list views; eg how you can press down, hold, and then release to trigger a row selection.
Or put another way, this mimics the row selection behavior UICollectionView provides.
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.
(And it does recognize a tap, since I set the min duration to 0:)
self.pressRecognizer = PressGestureRecognizer()
self.pressRecognizer.minimumPressDuration = 0.0
self.pressRecognizer.allowableMovementAfterBegin = 5.0
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.
Ah gotcha, just noticed the minimumPressDuration
set to 0 above - missed that before
{ | ||
let view = self.collectionView! | ||
|
||
self.layout.positionStickySectionHeadersIfNeeded(in: view) |
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.
Curious why you moved this out of updateLayout
- is there a case where you'd update the layout with positioning the headers?
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.
Since this was getting called in multiple layouts, I figured it made sense to centralize it here; which seemed to make sense.
} | ||
|
||
let kind : SupplementaryKind | ||
let layout : HeaderFooterLayout | ||
let measurer : (Sizing.MeasureInfo) -> CGSize | ||
|
||
let isPopulated : Bool | ||
|
||
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.
Doesn't actually matter ofc, but there's a checkbox in Xcode to strip whitespace from empty lines to prevent this in diffs :)
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.
Huh, TIL!
Co-authored-by: Kyle Bashour <[email protected]>
@@ -95,16 +95,24 @@ extension PresentationState | |||
cache.push(view, with: self.model.reuseIdentifier) | |||
} | |||
|
|||
func createReusableHeaderFooterView(frame : CGRect) -> UIView |
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.
Unused
This PR introduces support for tappable headers and footers, by adding an
onTap
handler toHeaderFooter
. When provided, the header becomes tappable, and you can provide apressedBackgroundView
to appear, similar to theselectedBackgroundView
of cells.There's some unrelated changes to how zIndexing is done to improve determinism in layout, and there are also some fixes to bugs I found alongside this work.