Feature/scroll to view controller #19

AndreasThenn · 2017-03-10T15:01:46Z

No description provided.

…oller

ADD support for animated flag ADD started with writing Snapshot test - WIP ADD Snapshot for Snapshot test, but current snapshot is not waht we want

ADD snapshot for tests MOD made scroll into view test work

…endent of snapshot check

MOD changed behavior of scrollToViewController to respect contentSize as scroll limit

AndreasThenn · 2017-03-10T15:04:50Z

Tests/StackClusterTests.swift

+                let targetVC = childStack?.childViewControllers[2]
+
+                // TODO: WIP stack layout is not as expected
+                it("a target should be able to be centered") {


@MP0w @ChristianOrgler Can someone please take a look? It seems like the nested StackView has messed up sizes, that's why scrolling fails.

in which test? can't you take a look yourself? :)

@ChristianOrgler I looked into it and for me it seems like the StackViewController has a bug in this nested case or my test setup might need some adaptions to set the scroll views size correctly. It would cost me too much time to fix the StackViewController and is not in the scope of the scroll to ViewController feature.

runtasticBot · 2017-03-10T15:17:48Z

	1 Warning
⚠️	Big PR

SwiftLint found issues

Warnings

File	Line	Reason
Constraint.swift	85	Force unwrapping should be avoided.
Constraint.swift	62	Function should have complexity 10 or less: currently complexity equals 19
Constraint.swift	62	Function body should span 40 lines or less excluding comments and whitespace: currently spans 74 lines
Constraint.swift	218	Returning Void in a function declaration is redundant.
Constraint.swift	221	Returning Void in a function declaration is redundant.
Constraint.swift	224	Returning Void in a function declaration is redundant.
Constraint.swift	227	Returning Void in a function declaration is redundant.
Constraint.swift	230	Returning Void in a function declaration is redundant.
Constraint.swift	233	Returning Void in a function declaration is redundant.
Constraint.swift	236	Returning Void in a function declaration is redundant.
Constraint.swift	36	Variable name should be between 3 and 40 characters long: 'to'
Constraint.swift	63	Variable name should be between 3 and 40 characters long: 'to'
Constraint.swift	144	Line should be 120 characters or less: currently 124 characters
Constraint.swift	242	Line should be 120 characters or less: currently 150 characters
Constraint.swift	246	Line should be 120 characters or less: currently 130 characters
Constraint.swift	268	Line should be 120 characters or less: currently 127 characters
Constraint.swift	271	Line should be 120 characters or less: currently 184 characters
Constraint.swift	272	Line should be 120 characters or less: currently 136 characters
ConstraintMakerExtendable.swift	30	Limit vertical whitespace to a single empty line. Currently 2.
ConstraintMakerPriortizable.swift	30	Limit vertical whitespace to a single empty line. Currently 2.
ConstraintPriority.swift	65	Operators should be surrounded by a single whitespace when defining them.
ConstraintPriority.swift	30	Limit vertical whitespace to a single empty line. Currently 2.
ConstraintPriority.swift	31	Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.
ConstraintViewDSL.swift	30	Limit vertical whitespace to a single empty line. Currently 2.
ConstraintViewDSL.swift	72	Variable name should be between 3 and 40 characters long: 'contentCompressionResistanceHorizontalPriority'
ConstraintViewDSL.swift	81	Variable name should be between 3 and 40 characters long: 'contentCompressionResistanceVerticalPriority'

Errors

File	Line	Reason
Constraint.swift	94	Prefer checking `isEmpty` over comparing `count` to zero.
Constraint.swift	136	Prefer checking `isEmpty` over comparing `count` to zero.

Generated by 🚫 Danger

codecov-io · 2017-03-10T15:21:03Z

Codecov Report

Merging #19 into master will decrease coverage by 1.63%.
The diff coverage is 87.75%.

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
- Coverage   99.33%   97.69%   -1.64%     
==========================================
  Files           9        9              
  Lines         300      347      +47     
==========================================
+ Hits          298      339      +41     
- Misses          2        8       +6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec1c743...7c547c6. Read the comment docs.

ChristianOrgler · 2017-03-10T16:49:58Z

Source/Views/StackViewController.swift

@@ -9,6 +9,12 @@
 import UIKit
 import SnapKit

+public enum RelativePosition {
+    case beginning


add some docu what does this actually mean and how it behaves if there is nothing to scroll or not able to scroll the child to the very top (or left) depending on the orientation.

+1, public stuff should always have documentation

ChristianOrgler · 2017-03-10T16:50:14Z

Source/Views/StackViewController.swift

@@ -24,7 +30,7 @@ final class StackViewController: UIViewController {
        self.preserveParentWidth = configuration.preserveParentWidth

        super.init(nibName: nil, bundle: nil)
-
+        


try to avoid these next time ;)

I guess we removed trailing space from swiftlint and @AndreasThenn has a different Xcode setting

ChristianOrgler · 2017-03-10T16:53:26Z

Source/Views/StackViewController.swift

+    ///   - relativePosition: An optional parameter to specify the alignement of the target. Depending of
+    ///     the stackViews' orientation some relativePosition values have different meaning: 
+    ///     beginning - top/left, end - bottom/right
+    public func scrollToViewController(target targetViewController: UIViewController,


this is not a really swifty naming.

I would suggest something like
func scroll(to viewController: UIViewController, at position: RelativePosition = .center, animated: Bool = true)

which results in the usage as
stack.scroll(to: child, at: .beginning)

and why is the default not animating it?

@ChristianOrgler I will adapt it, to use your naming, thx.
Why should the default be to animate it? - The only reason for no animation for me was to be more defensive: Animation is additional functionality. I am happy to set it to true if there's a valid reason.

ChristianOrgler · 2017-03-10T16:53:44Z

Source/Views/StackViewController.swift

+    public func scrollToViewController(target targetViewController: UIViewController,
+                                       position relativePosition: RelativePosition = .center, animated: Bool = false) {
+
+        guard self.childViewControllers.contains(targetViewController) else {


no self. needed

ChristianOrgler · 2017-03-10T16:59:53Z

Source/Views/StackViewController.swift

+        if stackView.axis == .vertical {
+            // keep current horizontal offset
+            targetOffset.x = -scrollView.contentInset.left
+            let topOffset = targetFrame.origin.y - scrollView.contentInset.top


move the calculation in the respected case

ChristianOrgler · 2017-03-10T17:03:35Z

Source/Views/StackViewController.swift

+            if targetOffset.y < scrollView.contentInset.top {
+                targetOffset.y = scrollView.contentInset.top
+            }
+            targetOffset.y = ensureRangeFor(value: targetOffset.y,


this is already done above right??

ChristianOrgler · 2017-03-10T17:03:53Z

Source/Views/StackViewController.swift

+                                            upperLimit: verticalLimit)
+        } else {
+            // keep current vertical offset
+            targetOffset.y = -scrollView.contentInset.top


why negative?

@ChristianOrgler I used the contentInset, there a positive value would move the scroll view out of the view. I improved the code now, by using the actual currentOffset.

ChristianOrgler · 2017-03-10T17:04:14Z

Source/Views/StackViewController.swift

+        } else {
+            // keep current vertical offset
+            targetOffset.y = -scrollView.contentInset.top
+            let leftOffset = targetFrame.origin.x - scrollView.contentInset.left


same as vertical, move it in the cases

@ChristianOrgler Why? I then I need to duplicate code, that I need in multiple cases.

ChristianOrgler · 2017-03-10T17:05:30Z

Tests/StackClusterTests.swift

+                    expect(vc).to(haveValidSnapshot())
+                }
+
+                it("a leftmost target gets left aligned, if it could not get right aligned correctly") {


I don't get this naming ^^

me too, please use something like:
context when ....... (if needed)
it should.....

ChristianOrgler · 2017-03-10T17:07:21Z

Tests/StackClusterTests.swift

+                let targetVC = childStack?.childViewControllers[2]
+
+                // TODO: WIP stack layout is not as expected
+                it("a target should be able to be centered") {


in which test? can't you take a look yourself? :)

joanromano

Could you please add a general description of the PR? What is it for? Is this to address deep linking?

I feel like this is a specific scenario of the issue how to show/navigate to a component which we try to address with deep linking, but not sure. In that case, we should maybe address this in a different way which is more generic to all Components and thus tends to an API which is easier to use and understand.

Don't get me wrong, but I don't really see how adding this public method in the StackViewController will ease the usage of the library (maybe there is something I am missing or overlooking and thus a general description of the PR would help).

ps: also as general rule of thumb, we should always try to avoid a significant decrease on code coverage and public documentation.

joanromano · 2017-03-11T09:19:47Z

Source/Views/StackViewController.swift

+            return
+        }
+
+        var targetOffset = CGPoint(x: 0, y: 0)


Or CGPoint.zero

or .zero ;)

@ChristianOrgler .zero does not work, var needs a contextual type.

joanromano · 2017-03-11T09:23:26Z

Source/Views/StackViewController.swift

@@ -69,4 +75,80 @@ final class StackViewController: UIViewController {
        stackView.addArrangedSubview(childViewController.view)
        childViewController.didMove(toParentViewController: self)
    }
+
+    /// A method to access the scrollView from outside and scroll to a given target ViewController. 


Is not really accessing the scroll view from outside, is it?

@joanromano Why not? The stackView is private and we want to access its scrolling functionality via code from the app. Its not complete access if that's your concern, I will then use a better description.

MP0w · 2017-03-11T09:42:24Z

Source/Views/StackViewController.swift

        scrollView.backgroundColor = configuration.backgroundColor
    }

-    required init?(coder aDecoder: NSCoder) {
+    required init?(coder _: NSCoder) {


reverted, was due SwiftFormat

MP0w · 2017-03-11T09:42:42Z

Source/Views/StackViewController.swift

@@ -48,13 +54,13 @@ final class StackViewController: UIViewController {
        scrollView.alwaysBounceVertical = false
        scrollView.showsHorizontalScrollIndicator = false

-        scrollView.snp.makeConstraints { (make) in
+        scrollView.snp.makeConstraints { make in


why these 2 changes?

@MP0w sorry, I have a code format plugin installed. (https://github.com/nicklockwood/SwiftFormat)
I tried to only use it on my code, but I case I used it on the whole file.
In our codebase we would remove the brackets, if they are not needed. I can revert these changes, if you want to.

reverted, was due SwiftFormat

joanromano · 2017-03-11T09:41:38Z

Source/Views/StackViewController.swift

@@ -9,6 +9,12 @@
 import UIKit
 import SnapKit

+public enum RelativePosition {
+    case beginning


+1, public stuff should always have documentation

joanromano · 2017-03-11T09:42:30Z

Source/Views/StackViewController.swift

+            targetOffset.x = ensureRangeFor(value: targetOffset.x,
+                                            lowerLimit: scrollView.contentInset.left,
+                                            upperLimit: horizontalLimit)
+        }


Had a general look here and also seems to me that some ensureRangeFor calls are not needed.

joanromano · 2017-03-11T09:44:30Z

Tests/StackClusterTests.swift

+                }
+            }
+
+            context("when scrolling horziontally in a vertical nested stack") {


horizontally

joanromano · 2017-03-11T09:46:39Z

Tests/StackClusterTests.swift

@@ -161,7 +163,7 @@ class StackClusterTests: QuickSpec {
                    it("should be able to scroll horizontally") {
                        let vc = nestedStack()
                        vc?.loadViewIfNeeded()
-
+                        vc?.view.layoutIfNeeded()


Why do we need those layoutIfNeeded here?

@joanromano I added it to every testcase, that would fail, if the check with haveValidSnapshot was not called. Checking for a snapshot forces the layout, but if you remove the check and then just check for expect(scrollView).to(scroll(.horizontal)) the test would fail.

so why is it added to some tests which have expect(vc).to(haveValidSnapshot()) as the first check? Sorry I may not understand you 😕

It's probably because you ask to load the view but nothing triggers a layout. You don't need to load the view before doing a snapshot and you don't need to check here if it can scroll since there are already tests for this (if I missed some than add the tests cases but don't mix it with these unrelated specs)

I think the reason was that nothing is triggering a layout action (because it's not embeded somewhere) - therefore the layoutIfNeeded is needed here, right @AndreasThenn ?

@ChristianOrgler is right. @joanromano @MP0w you don't need to load the view to take snapshots, that's true. But you have three expectations: expect(vc).to(haveValidSnapshot()) expect(scrollView).to(scroll(.horizontal)) expect(scrollView).toNot(scroll(.vertical))
In my opinion these expectations should be independent and the order should not matter. Having to check haveValidSnapshot() should not be a precondition for the other two expectations.

you don't need to check here if it can scroll

I did not add these, I just fixed that they will succeed, even without checking for a snapshot.

MP0w · 2017-03-11T10:18:29Z

We need some explanation why we want to add this, because currently this StackVC can't be accessed from user of Matrioska AFAIK (they just get a Component) so this method can't even be used... it's hard to understand why we want to add this

AndreasThenn · 2017-03-11T15:49:50Z

After we got our AppStructure built we have all the ViewControllers we need. In our case, a UITabBarController with some children. Some of these children are of type StackViewController, so we can access them and we need to, because we want to scroll to some view controllers via code. The scrollViewof the StackViewControlleris currently private, so we can not access this one. But we want other developers to be able to do this, too.

joanromano · 2017-03-11T16:17:32Z

@AndreasThenn afaik and as @MP0w said StackViewController is an internal class so clients are not able to use it, so the new method added in your implementation is not accessible, right?

But also, I think a specific use case should not drive the implementation. I would expect Matrioska to support navigation to some Component or child Component in a generic way. Thus, adding a concrete method on the StackViewController class is not properly addressing the feature that we want to implement and feels somehow incomplete/wrong imo.

I believe we should let all the Component which are not single opt in to support navigation and then implement it in the ones that we provide, recursively (Stack scrolls to specific childs, TabBar selects the proper tab and recursively searches for the child, etc).

ChristianOrgler · 2017-03-12T12:01:44Z

@joanromano @MP0w Yes this is driven by the fact that we need this for deep linking – when we know where we want, we somehow want to highlight and scroll to a specific child.

I'm on the idea to have a generic way of doing this. E.g. a protocl Navigate ? But is this really a thing in Matrioska or in the app extending the given VCs from Matrioska.. (StackVC & TabbarVC)
Assuming an app already has a deep linking system to navigate somewhere.

We could make the StackViewController extendable then we don't need to care about this in the project itself which should maybe only be responsible for building UI dynamically. :-)

What do you think?

joanromano · 2017-03-13T09:15:03Z

@ChristianOrgler afaik clients should, ideally, only know about Components so not know about a StackViewController. If they want to navigate to specific Component there should just be a public api for that in Component, right?

ChristianOrgler · 2017-03-13T12:39:34Z

@joanromano but this needs to be available on run-time right? So how does a client access the component? The Component is only accessible during the build-UI part right?

Can you give me a more concrete example on how the flow should look like form your point of view?

joanromano · 2017-03-13T13:11:16Z

@ChristianOrgler maybe I am missing something but a client has normally access to the root Component, right? A client then should be able to ask to this root Component (or whatever component, actually), for a recursive search and show of a specific Component.

Not quite sure what you mean by this needs to be available on run-time, sorry.

ChristianOrgler · 2017-03-13T13:51:25Z

Currently I don't see a possibility to access the Component after the UI is built and shown, right?

But when a user taps on a view which may redirect him to another view in another tab or so, I need to be able to call a method on the Tabbar-VC to do so. (or the StackView in that case)

Can you give me an example how this should be done right now?

joanromano · 2017-03-13T14:07:47Z

Currently I don't see a possibility to access the Component after the UI is built and shown, right?

Why? e.g: https://github.com/runtastic/Matrioska/blob/master/Example/MatrioskaExample/AppDelegate.swift

There the root and all his child components are accessible aren't they?

But when a user taps on a view which may redirect him to another view in another tab or so, I need to be able to call a method on the Tabbar-VC to do so. (or the StackView in that case)

True, but I don't think it makes sense to do this on a VC basis. If we provide more custom VCs to provide more out of the box Clusters (say, for instance, a hamburger menu cluster) we would always have to have different APIs for the different VCs, making it really hard and confusing to use. We should do this on a generic Component api.

I believe there must be a way to do this, e.g we should create a protocol/method that is optionally opt-in for Components in order to navigate/show a child by a given id.

protocol Foo {
   func showChild(with id: id)
}

Which then Stack or TabBar clusters would implement differently inside. Sorry I have no time for a proper implementation.

If you guys really think this is not possible or makes no sense maybe you are right, but from the PR seems like is not really correct conceptually speaking what is done here, since this is only a new method added to the StackVC just to serve a specific use case from the whole issue, but not useful for the library at all (I can't create nested clusters and use this for instance, or the StackVC itself is internal so is not even accessible).

As said got no time now for a real implementation but I will give it a try later on.

ChristianOrgler · 2017-03-13T17:52:38Z

@joanromano as I said "I'm on the idea to have a generic way of doing this. E.g. a protocl Navigate ?"

I also think we need a more generic approach to this. Open up the StackVC and Tabbar and whatever we provide here and create a protocol for this. 👍

@AndreasThenn can you introduce a protocol with proper naming and implementation in this PR?

MOD improved naming of StackClusterTests REM old test images ADD new snaphot images to fit new naming

ADD protocol Focusable to UITabBarController and StackViewController

AndreasThenn · 2017-03-14T17:32:28Z

@ChristianOrgler I updated my PR and introduced Focusable as protocol. No tests yet, but I want to wait for feedback first.

MOD made scroll method internal

FIX StackViewController now keeps stackView height when horizontal aligned, this improves nested views. REM old reference images MOD renamed StackClusterTests

ktraunmueller · 2017-03-15T09:33:59Z

The build bot does not pick up the SwiftLint configuration file in the project? (complains about violations in Constraint.swift, which is in Pods/SnapKit)

ktraunmueller · 2017-03-15T09:42:04Z

Source/Protocols/Focusable.swift

+protocol Focusable {
+
+    /** Try to focus on the given UIViewController, otherwise it fails silently */
+    @discardableResult


@discardableResult on a function with return type Void is kind of pointless, wouldn't you agree?

ktraunmueller · 2017-03-15T10:11:27Z

Source/Protocols/Focusable.swift

+
+    /** Try to focus on the given UIViewController, otherwise it fails silently */
+    @discardableResult
+    func focus(on viewController: UIViewController)


I second @joanromano's position: "I would expect Matrioska to support navigation to some Component or child Component in a generic way". Matrioska is basically a "Component tree API", so the API should allow clients to focus Components, not UIViewControllers.

But before that, I think we need to introduce an identity concept for Components, a means by which Components can be looked up, so that you could find the Component to focus.

The question is also whether the focus request should be dispatched to the root of the Component tree, i.e.

root.focus(componentId: someId)

or the Component to focus:

if let someNestedComponent = root.find(by: someId) { someNestedComponent.focus() }

To me it looks like the focusing will require a top-down approach anyway (stuff higher up the hierarchy potentially needs to be scrolled into position before focusing a nested child), so the first alternative seems the more natural thing to do.

AndreasThenn · 2017-03-17T11:21:32Z

As discussed offline, we will implement this in the app and not in Matrioska.

AndreasThenn added 10 commits March 8, 2017 16:59

ADD support for StackViewController to scroll into a child view contr…

7c428d5

…oller

ADD description

acc5db1

CRC

29fadc1

CRC

60eba94

FIX calulcation of targetOffset

321d7a3

ADD support for animated flag ADD started with writing Snapshot test - WIP ADD Snapshot for Snapshot test, but current snapshot is not waht we want

REM unused snapshot

526159d

ADD snapshot for tests MOD made scroll into view test work

ADD call view.layoutIfNeeded() where needed, to make expections indep…

5ca2917

…endent of snapshot check

MOD updated StackClusterTests

80eb8d6

FIX changed wrong recordSnapshot to correct haveValidSnapshot call

a410445

ADD more scrollToViewController tests

c2c0399

MOD changed behavior of scrollToViewController to respect contentSize as scroll limit

AndreasThenn requested review from MP0w and ChristianOrgler March 10, 2017 15:02

AndreasThenn commented Mar 10, 2017

View reviewed changes

AndreasThenn requested review from ktraunmueller and mathiasAichinger March 10, 2017 15:06

ChristianOrgler suggested changes Mar 10, 2017

View reviewed changes

joanromano suggested changes Mar 11, 2017

View reviewed changes

MP0w reviewed Mar 11, 2017

View reviewed changes

joanromano reviewed Mar 11, 2017

View reviewed changes

joanromano requested a review from MP0w March 11, 2017 10:21

AndreasThenn added 4 commits March 14, 2017 10:21

CRC

e63e952

FIX use current offset of scrollView instead of contentInset values

6fc3328

MOD renamed beginning to just begin

f2e455d

MOD improved naming of StackClusterTests REM old test images ADD new snaphot images to fit new naming

ADD protocol to encapsulate the actual select, scroll method

4fd8a01

ADD protocol Focusable to UITabBarController and StackViewController

AndreasThenn added 2 commits March 15, 2017 06:56

MOD updated documentation

280625d

MOD made scroll method internal

FIX build by calling pod update on the example

c31c61d

FIX StackViewController now keeps stackView height when horizontal aligned, this improves nested views. REM old reference images MOD renamed StackClusterTests

ktraunmueller reviewed Mar 15, 2017

View reviewed changes

CRC

7c547c6

AndreasThenn closed this Mar 17, 2017

		@@ -24,7 +30,7 @@ final class StackViewController: UIViewController {
		self.preserveParentWidth = configuration.preserveParentWidth

		super.init(nibName: nil, bundle: nil)

Feature/scroll to view controller #19

Feature/scroll to view controller #19

Conversation

AndreasThenn commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runtasticBot commented Mar 10, 2017 • edited

SwiftLint found issues

Warnings

Errors

codecov-io commented Mar 10, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joanromano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MP0w commented Mar 11, 2017

AndreasThenn commented Mar 11, 2017

joanromano commented Mar 11, 2017 • edited

ChristianOrgler commented Mar 12, 2017

joanromano commented Mar 13, 2017

ChristianOrgler commented Mar 13, 2017

joanromano commented Mar 13, 2017

ChristianOrgler commented Mar 13, 2017

joanromano commented Mar 13, 2017

ChristianOrgler commented Mar 13, 2017

AndreasThenn commented Mar 14, 2017

ktraunmueller commented Mar 15, 2017

Choose a reason for hiding this comment

ktraunmueller Mar 15, 2017 • edited

Choose a reason for hiding this comment

AndreasThenn commented Mar 17, 2017

runtasticBot commented Mar 10, 2017 •

edited

codecov-io commented Mar 10, 2017 •

edited

joanromano commented Mar 11, 2017 •

edited

ktraunmueller Mar 15, 2017 •

edited