Added Restricted Brick Size #168
Conversation
|
Please add a description on what this PR does |
Source/Models/BrickDimension.swift
Outdated
| case maximumSize(size: BrickDimension) | ||
| } | ||
|
|
||
| public struct RestrictedBrickSize { |
There was a problem hiding this comment.
Why is this a separate struct and not just part of the BrickSize restricted? I'd had expected a new case in the enum indirect case restricted(size: BrickDimension, restriction: RestrictedBrickSize)
Source/Models/BrickDimension.swift
Outdated
| let restrictedSize: RestrictedSize | ||
|
|
||
| var isEstimate: Bool { | ||
| switch size { |
There was a problem hiding this comment.
The switch should be done on size.dimension(withValue: value), because this could be an indirect size
Source/Models/BrickDimension.swift
Outdated
| var isEstimate: Bool { | ||
| switch size { | ||
| case .auto(_): return true | ||
| default: return false |
There was a problem hiding this comment.
Thats not really applicable, if you're not using auto there is no point of a restriction
There was a problem hiding this comment.
We should still write a test to document this
| DummyResizableBrick("Brick", height: .restricted(restriction: RestrictedBrickSize(size: .auto(estimate: .fixed(size: 100)), restrictedSize: .minimumSize(size: .fixed(size: 100)))), setHeight: 200) | ||
| ]) | ||
|
|
||
| brickCollectionView.setupSectionAndLayout(section) |
There was a problem hiding this comment.
You can also use setupSingleBrickAndLayout. That way you don't have to wrap it in a BrickSection
|
|
||
| brickCollectionView.setupSectionAndLayout(section) | ||
|
|
||
| let cell = brickCollectionView.cellForItem(at: IndexPath(item: 0, section: 1)) as? DummyResizableBrickCell |
There was a problem hiding this comment.
Check the indexpath by calling brickCollectionView.indexPathsForBricksWithIdentifier. It's a safer way to get to the right cell
| let cell = brickCollectionView.cellForItem(at: IndexPath(item: 0, section: 1)) as? DummyResizableBrickCell | ||
| cell?.layoutIfNeeded() | ||
| XCTAssertEqual(cell?.frame.size.height, 50) | ||
| } |
There was a problem hiding this comment.
As RestrictedBrickSize takes in a BrickDimension for restrictedSize, there should be a check for using other BrickDimensions as well, like Ratio, Fixed + indirect ones (like Size dependent etc)
There was a problem hiding this comment.
Using anything else will cause a fatalError on BrickDimension.swift:175 fatalError("Only Ratio, Fixed and Fill are allowed") as designed
There was a problem hiding this comment.
It should still work with indirect dimensions. The error fatalError("Only Ratio, Fixed and Fill are allowed") should never happen
jay18001
force-pushed
the
restricted-brick-size
branch
from
41882bd
to
97529c0
Compare
Codecov Report
@@ Coverage Diff @@
## master #168 +/- ##
==========================================
+ Coverage 93.34% 93.37% +0.02%
==========================================
Files 39 39
Lines 4134 4149 +15
Branches 335 339 +4
==========================================
+ Hits 3859 3874 +15
Misses 274 274
Partials 1 1
Continue to review full report at Codecov.
|
| return "Basic example of using bricks with a restricted maximum and minimum height" | ||
| } | ||
|
|
||
| override func viewDidLoad() { |
There was a problem hiding this comment.
Can we also add an example that changes from .restricted > .auto to simulate the expand/collapse behavior?
Source/Models/BrickDimension.swift
Outdated
| internal static func restrictedValue(for cellHeight: CGFloat, width: CGFloat, restrictedSize: RestrictedSize) -> CGFloat { | ||
| switch restrictedSize { | ||
| case .minimumSize(let minimumDimension): | ||
| return max(cellHeight, BrickDimension._rawValue(for: width, startingAt: 0, with: minimumDimension)) |
There was a problem hiding this comment.
We should pass the returned size from the dimension-function instead, to support indirect sizes as well
There was a problem hiding this comment.
We could do that but then the user of this would have to know that .auto cannot be used anywhere else
| XCTAssertEqual(cell?.frame.size.height, 100) | ||
| } | ||
|
|
||
| func testChangeBrickSizeLessThanMaximum() { |
There was a problem hiding this comment.
I want to see more tests where the restrictedSize is not .fixed
There was a problem hiding this comment.
There should also be indirect-cases (based on orientation, size class etc)
jay18001
force-pushed
the
restricted-brick-size
branch
5 times, most recently
from
c018e47
to
5807f71
Compare
jay18001
force-pushed
the
restricted-brick-size
branch
from
5807f71
to
3025824
Compare
Created a way for bricks to be restricted with a minimum size or maximum size