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

Allow panel to be added to a subview of the view controller #40

Closed
wants to merge 3 commits into from
Closed

Allow panel to be added to a subview of the view controller #40

wants to merge 3 commits into from

Conversation

futuretap
Copy link
Contributor

@futuretap futuretap commented Nov 9, 2018

Allow panel to be added to a subview of the view controller instead of the root view of the vc.

I added an optional containerView parameter to the addPanel func. If set, the panel is added to that containerView instead of viewController.view.

The sample app was updated to showcase the feature, using a containerView that is placed above a toolBar.

This requires iOS 11+ because safeAreaInsets aren't available on the view level in older iOS versions.

@scenee
Copy link
Owner

scenee commented Nov 12, 2018

Thank you for your PR and contribution😆 I'm glad them 👍

I reviewed your PR and considered what is best.

In conclusion, I can't merge it to master because I think it's better to resolve the container issue on the user side. Instead of this change to this library, you can use a floating panel by defining a view controller for a container view. It's easy and make this library clear and simple.

Here is a patch for the solution sample.
0001-Use-SubviewContainerViewController.patch.tar.gz

And this PR has some problems.

I wouldn't like to drop the iOS10 support for now because it's possible.

I hope you would accept my decision.

Finally, I really appreciate your contribution. Thanks again.

@scenee scenee closed this Nov 12, 2018
@futuretap
Copy link
Contributor Author

I'm sorry, I can't get the patch to apply cleanly. Can you post a full tarball link or push a branch?

@futuretap
Copy link
Contributor Author

futuretap commented Nov 12, 2018

OK, managed to construct it manually.

This solution works but unfortunately it has significant consequences regarding the view controller containment structure.

I have a master view controller with the toolbar and a bunch of other child view controllers (let's call them "ABC") managing certain parts of the view. With your proposed solution, I have to move all "ABC" child view controllers one level deeper as subVCs of the panelContainerVC in order to be able to attach the panel to the root view of the panelContainerVC.

Before:

  • rootVC
    • A
    • B
    • C
    • Panel
      • Panel Content

After:

  • rootVC
    • panelContainerVC
      • A
      • B
      • C
      • Panel
        • Panel Content

This is certainly doable but it strikes me as odd to move all content VCs one VC level deeper just because I add a panel to the rootVC.

@scenee
Copy link
Owner

scenee commented Nov 12, 2018

I'm sorry, I can't get the patch to apply cleanly. Can you post a full tarball link or push a branch?

Could you apply the above patch to the master 72f5d59? The patch occurs "warning: 1 line adds whitespace errors.", but it can be ignored 🙂
If you care of it, the new one is free from the warning.
0001-Use-SubviewContainerViewController.patch.tar.gz


EDIT

I'm sorry I found a storyboard problem in the above patch. please use this fixed patch.
0001-Use-SubviewContainerViewController-fix.patch.tar.gz

@scenee
Copy link
Owner

scenee commented Nov 12, 2018

I have a master view controller with the toolbar and a bunch of other child view controllers (let's call them "ABC") managing certain parts of the view.

By the way, why not add the floating panel directly to the root VC?
If you would like to display a floating panel on a tool bar, you have a solution without using a container view 😄 I attach additional patch to 0001-Use-SubviewContainerViewController-fix.patch.tar.gz.
0001-Add-floating-panel-directly.patch.tar.gz

@futuretap
Copy link
Contributor Author

Can you please advise how to apply the patch?

I checked out master (72f5d59) and tried like this:

patch -p1 <../0001-Add-floating-panel-directly.patch
patching file Examples/Samples/Sources/Base.lproj/Main.storyboard
Hunk #1 succeeded at 556 (offset 12 lines).
Hunk #2 FAILED at 565.
1 out of 2 hunks FAILED -- saving rejects to file Examples/Samples/Sources/Base.lproj/Main.storyboard.rej
patching file Examples/Samples/Sources/ViewController.swift
Hunk #1 FAILED at 500.
Hunk #2 FAILED at 517.
Hunk #3 FAILED at 533.
3 out of 3 hunks FAILED -- saving rejects to file Examples/Samples/Sources/ViewController.swift.rej

@futuretap
Copy link
Contributor Author

Tried to apply the patch using manual edits. It seems to work, only the bottom inset of the content scrollview is not adjusted correctly. Looking at the code, it seems like additionalSafeAreaInsets of the content view controller (or FloatingPanelController) are not respected because the scrollView's contentInset is set manually. Can you advise how to fix the bottom contentInset?

@scenee
Copy link
Owner

scenee commented Nov 13, 2018

Can you please advise how to apply the patch?

Could you try the following commands?

$ git apply 0001-Use-SubviewContainerViewController-fix.patch
$ # And then
$ git apply 0001-Add-floating-panel-directly.patch

Can you advise how to fix the bottom contentInset?

You can set .never to FloatingPanelController.contentInsetAdjustmentBehavior and set a custom content insets to a scroll view referring to adjustedContentInsets.
By default, contentInsetAdjustmentBehavior is .always. So a tracking scroll view's content offsets is equal to adjustedContentInsets.
I hope it could help you.

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.

None yet

2 participants