Varadic range support #140

vlozko · 2017-06-29T18:00:35Z

Allows the BrickKit collection view controller to apply different dimensions based upon how wide the brick is. Starting with a minimum dimension that would be applicable to all widths, the app can the specify different dimensions for any additional widths.

100% code coverage for all additional code.
Added code to sample app to demonstrate support for it.

… sorting them internally.

thevwu

Changes to how we call the new dimensionRange

thevwu · 2017-06-29T18:09:40Z

Source/Models/BrickDimension.swift

@@ -18,39 +18,106 @@ public struct BrickSize {
    }
 }

+public typealias RangeDimensionPair = (dimension: BrickDimension, minimumLength: CGFloat)
+
+public struct BrickRangeDimention {


should be Dimension

thevwu · 2017-06-29T18:11:36Z

Source/Models/BrickDimension.swift

    indirect case auto(estimate: BrickDimension)
    indirect case orientation(landscape: BrickDimension, portrait: BrickDimension)
    indirect case horizontalSizeClass(regular: BrickDimension, compact: BrickDimension)
    indirect case verticalSizeClass(regular: BrickDimension, compact: BrickDimension)
-
+    indirect case dimensionRange(range: BrickRangeDimention)


ideally this would be

indirect case dimensionRange(minimum: BrickDimension, additionalRangePairs: [RangeDimensionPair]?)

thevwu · 2017-06-29T18:12:54Z

Source/Models/BrickDimension.swift

        case .verticalSizeClass(let regular, let compact):
            let isRegular = BrickDimension.verticalInterfaceSizeClass == .regular
-            return (isRegular ? regular : compact).dimension
+            return (isRegular ? regular : compact).dimension(withValue: value)
+        case .dimensionRange(let dimensionRange):


With the change to the indirect case, you now initialize the BrickRangeDimension here instead

thevwu · 2017-06-29T18:15:01Z

Tests/Models/BrickDimensionTests.swift

+        let additionalRangePairs1: [RangeDimensionPair] = [(dimension: .ratio(ratio: 1/2), minimumLength: CGFloat(350))]
+        let dimensionRange1: BrickDimension = .dimensionRange(range: BrickRangeDimention(minimum: .ratio(ratio: 1.0), additionalRangePairs: additionalRangePairs1))
+        let additionalRangePairs2: [RangeDimensionPair] = [(dimension: .ratio(ratio: 0.5), minimumLength: CGFloat(350))]
+        let dimensionRange2: BrickDimension = .dimensionRange(range: BrickRangeDimention(minimum: .ratio(ratio: 0.5 + 0.5), additionalRangePairs: additionalRangePairs2))


Not a fan of calling enum .dimensionRange and then initializing an object that is BrickRangeDimension, provided comments on how to adjust this call so that it's similar to the other BrickDimension cases we have

codecov-io · 2017-06-29T18:59:26Z

Codecov Report

Merging #140 into master will increase coverage by 0.1%.
The diff coverage is 98.3%.

@@            Coverage Diff            @@
##           master     #140     +/-   ##
=========================================
+ Coverage   93.25%   93.35%   +0.1%     
=========================================
  Files          38       38             
  Lines        3097     3144     +47     
=========================================
+ Hits         2888     2935     +47     
  Misses        209      209

Impacted Files	Coverage Δ
Source/Cells/BrickCell.swift	`97.8% <100%> (ø)`	⬆️
Source/Bricks/Image/ImageBrick.swift	`77.98% <100%> (ø)`	⬆️
Source/Models/BrickDimension.swift	`99.09% <100%> (+0.62%)`	⬆️
...rs/BrickCollectionView+BrickLayoutDataSource.swift	`100% <100%> (ø)`	⬆️
Source/Bricks/Collection/CollectionBrick.swift	`84.31% <50%> (ø)`	⬆️
Source/ViewControllers/BrickCollectionView.swift	`95% <0%> (+0.03%)`	⬆️

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 183af9e...8b5a000. Read the comment docs.

rubencagnie · 2017-06-29T20:15:59Z

Source/Models/BrickDimension.swift

@@ -18,39 +18,106 @@ public struct BrickSize {
    }
 }

+public typealias RangeDimensionPair = (dimension: BrickDimension, minimumLength: CGFloat)


Is Length the correct term? Should we use minimumWidth?

I was uncertain about this one for a while, too. I wanted this to be applicable for height-based dimensions, too. However, I noticed in the code that we only support direct cases for height brick dimensions. I don't know if that's by design or a shortcoming. There's no good generic term that would encompass both width and height. For example, size is already reserved for a struct that contains both width and height.

We're already using size for .fixed(size: 200)? I think if we come up with the right terms, we should change it there too (might be some refactor on the WayfairApp though)...

rubencagnie · 2017-06-29T20:17:37Z

Source/Models/BrickDimension.swift

+
+    internal var dimensionPairs : [RangeDimensionPair]
+
+    public init(minimum dimension: BrickDimension, additionalRangePairs: [RangeDimensionPair]?) {


could we use default instead of minimum? Or fallBack?

rubencagnie · 2017-06-29T20:18:23Z

Source/Models/BrickDimension.swift

+
+    internal var dimensionPairs : [RangeDimensionPair]
+
+    public init(minimum dimension: BrickDimension, additionalRangePairs: [RangeDimensionPair]?) {


Could we make it non-optional and default it to []?

rubencagnie · 2017-06-29T20:19:22Z

Source/Models/BrickDimension.swift

+        }
+    }
+
+    public func dimension(forWidth width: CGFloat?) -> BrickDimension {


Why is width optional?

It returns the default/minimum one if one isn't provided.

rubencagnie · 2017-06-29T20:19:53Z

Source/Models/BrickDimension.swift

    indirect case auto(estimate: BrickDimension)
    indirect case orientation(landscape: BrickDimension, portrait: BrickDimension)
    indirect case horizontalSizeClass(regular: BrickDimension, compact: BrickDimension)
    indirect case verticalSizeClass(regular: BrickDimension, compact: BrickDimension)
-
+    indirect case dimensionRange(minimum: BrickDimension, additionalRangePairs: [RangeDimensionPair]?)
+
    public var isEstimate: Bool {


Could we pass the width here too?

Swift doesn't allow you to provide default arguments for an enum case.

could we make it a func instead?
public func isEstimate(withValue value: CGFloat) -> Bool

rubencagnie · 2017-06-29T20:20:19Z

Source/Models/BrickDimension.swift

        case .auto(_): return true
        default: return false
        }
    }

-    var dimension: BrickDimension {
+    func dimension(withValue value: CGFloat? = nil) -> BrickDimension {


can we make it non-optional?

The concern here is that only one case actually needs to use a value. We'd be forcing people to provide a completely unused value for all the other cases.

rubencagnie · 2017-06-30T15:03:07Z

Source/Models/BrickDimension.swift

+    return lhs.dimension == rhs.dimension && almostEqualRelative(first: lhs.minimumLength, second: rhs.minimumLength)
+}
+
+public func ==(lhs: BrickRangeDimension, rhs: BrickRangeDimension) -> Bool {


Is there no unit test for this?

There were but the last commit ended up modifying the coverage. I'll add additional tests to handle the additional cases.

…l unit tests for more complete code coverage.

jay18001 · 2017-07-05T14:55:51Z

Source/Models/BrickDimension.swift

        default: return self
        }
    }

    func value(for otherDimension: CGFloat, startingAt origin: CGFloat) -> CGFloat {
-        let actualDimension = dimension
+        let actualDimension: BrickDimension = dimension(withValue: otherDimension)


Do we need this type here?

It did at one point. I can remove it.

jay18001 · 2017-07-05T14:57:33Z

Can you add an example playground

Vadim Lozko added 2 commits June 29, 2017 13:47

Added support for dimensions based on the total width value.

9b78bd7

Added support for allowing the range pairs to be set out of order and…

ac7d7f5

… sorting them internally.

thevwu suggested changes Jun 29, 2017

View reviewed changes

Vadim Lozko added 2 commits June 29, 2017 15:02

Fixed spelling errors and adjusted enum case for dimension range.

c537943

Fixed up the sample app.

1da01fa

rubencagnie suggested changes Jun 29, 2017

View reviewed changes

rubencagnie reviewed Jun 30, 2017

View reviewed changes

Adjusted naming of parameters, method signatures, and added additiona…

eff1468

…l unit tests for more complete code coverage.

rubencagnie approved these changes Jul 5, 2017

View reviewed changes

jay18001 suggested changes Jul 5, 2017

View reviewed changes

Removed specifying of type. No longer necessary.

8b5a000

jay18001 approved these changes Jul 5, 2017

View reviewed changes

jay18001 merged commit 6b48463 into wayfair-archive:master Jul 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Varadic range support #140

Varadic range support #140

vlozko commented Jun 29, 2017

thevwu left a comment

thevwu Jun 29, 2017

thevwu Jun 29, 2017

thevwu Jun 29, 2017

thevwu Jun 29, 2017

codecov-io commented Jun 29, 2017 •

edited

Loading

rubencagnie Jun 29, 2017

vlozko Jun 30, 2017

rubencagnie Jun 30, 2017

rubencagnie Jun 29, 2017

rubencagnie Jun 29, 2017

rubencagnie Jun 29, 2017

vlozko Jun 30, 2017

rubencagnie Jun 29, 2017

vlozko Jun 30, 2017

rubencagnie Jun 30, 2017

rubencagnie Jun 29, 2017

vlozko Jun 30, 2017

rubencagnie Jun 30, 2017

vlozko Jun 30, 2017

jay18001 Jul 5, 2017

vlozko Jul 5, 2017

jay18001 commented Jul 5, 2017


		internal var dimensionPairs : [RangeDimensionPair]

		public init(minimum dimension: BrickDimension, additionalRangePairs: [RangeDimensionPair]?) {

Varadic range support #140

Varadic range support #140

Conversation

vlozko commented Jun 29, 2017

thevwu 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

codecov-io commented Jun 29, 2017 • edited Loading

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

jay18001 commented Jul 5, 2017

codecov-io commented Jun 29, 2017 •

edited

Loading