-
Notifications
You must be signed in to change notification settings - Fork 531
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
Add Horizontal Scroll Support #118
base: master
Are you sure you want to change the base?
Conversation
|
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.
Some minor style comments 🙂
@@ -440,7 +445,7 @@ private extension PanModalPresentationController { | |||
|
|||
guard | |||
let scrollView = presentable?.panScrollable, | |||
!scrollView.isScrolling | |||
!scrollView.isScrolling && presentable?.shouldConfigureScrollViewInsets == true |
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.
To keep the coding style consistent, I'd suggest
- replacing the
&&
operator with a,
like in the previous statement - remove the
== true
asshouldConfigureScrollViewInsets
is a boolean - giving this it's own line and not appending to the previous for better legibility
@@ -759,7 +764,8 @@ private extension PanModalPresentationController { | |||
Halts the scroll of a given scroll view & anchors it at the `scrollViewYOffset` | |||
*/ | |||
func haltScrolling(_ scrollView: UIScrollView) { | |||
scrollView.setContentOffset(CGPoint(x: 0, y: scrollViewYOffset), animated: false) | |||
guard presentable?.shouldHaltScroll != false else { return } |
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.
again, no need to check if something is not false.
My suggestion:
if presentable?.shouldHaltScroll { return }
which reads a bit easier 😉
scrollView.showsVerticalScrollIndicator = true | ||
scrollViewXOffset = max(scrollView.contentOffset.x, 0) | ||
|
||
if presentable?.shouldConfigureScrollViewInsets == true { |
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.
if presentable?.shouldConfigureScrollViewInsets
without the == true
Usage: | ||
``` | ||
extension YourViewController: PanModalPresentable { | ||
func shouldRoundTopCorners: Bool { return false } | ||
func shouldRoundTopCorners: Bool { return false } |
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.
Your IDE may have done this, but these whitespaces probably are deliberate and should remain in this comment 😄
Default value is .max. | ||
*/ | ||
var longFormHeight: PanModalHeight { get } | ||
|
||
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.
Looks like your IDE filled in whitespaces for no reason here. Double check if you Xcode trims these.
Xcode -> Preferences -> Text Editing -> Editing -> While Editing -> CheckAutomatically trim trailing whitespace
and Including whitespace-only lines
Summary
Currently PanModal only supports vertical scroll behavior, but there are cases where horizontal support may be needed, for example a horizontally scrolling set of images. This PR hopes to add that support to this library.
Requirements (place an
x
in each[ ]
)I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've written tests to cover the new code and functionality included in this PR.
Note: I'm not sure if this requires additional tests from what I could see.