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.

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

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

Conversation

dimitryvin
Copy link

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.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@jordanpichler jordanpichler left a 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
Copy link
Contributor

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 as shouldConfigureScrollViewInsets 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 }
Copy link
Contributor

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 {
Copy link
Contributor

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 }
Copy link
Contributor

@jordanpichler jordanpichler Oct 8, 2020

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 }

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants