-
Notifications
You must be signed in to change notification settings - Fork 57
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
Inserting/deleting items from middle of a brickCollectionView #149
Inserting/deleting items from middle of a brickCollectionView #149
Conversation
Codecov Report
@@ Coverage Diff @@
## master #149 +/- ##
=========================================
Coverage ? 93.49%
=========================================
Files ? 39
Lines ? 3181
Branches ? 0
=========================================
Hits ? 2974
Misses ? 207
Partials ? 0
Continue to review full report at Codecov.
|
} | ||
|
||
extension SegmentHeaderBrickDataSource { | ||
func configure(cell: SegmentHeaderBrickCell) {/*Optional*/} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an old brick. Ideally, we should use GenericBrick<UISegementedControl>
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to generic brick
return ["No animation", "From top", "From bottom", "Fade in"] | ||
} | ||
|
||
var insertToIndex: Int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable name is a bit confusing, feels more like a function name. Can you call it more indexToInsertTo
?
@@ -49,47 +67,38 @@ class InsertBrickViewController: BrickApp.BaseBrickController, HasTitle { | |||
self.navigationItem.rightBarButtonItem = UIBarButtonItem(barButtonSystemItem: .add, target: self, action: #selector(InsertBrickViewController.insertBrick)) | |||
|
|||
let section = BrickSection(Section, bricks: [ | |||
SegmentHeaderBrick(dataSource: self, delegate: self), | |||
LabelBrick(BrickIdentifiers.repeatLabel, backgroundColor: UIColor.lightGray, dataSource: self) | |||
BrickSection(bricks: [SegmentHeaderBrick("AppearSegmentHeaderBrick", dataSource: self, delegate: self), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make the string into constants?
Can you also align the bricks, so it's a bit more readable?
let section = BrickSection(Section, bricks: [
BrickSection(bricks: [
SegmentHeaderBrick("AppearSegmentHeaderBrick", dataSource: self, delegate: self),
SegmentHeaderBrick("InsertSegmentHeaderBrick", dataSource: self, delegate: self)
]),
LabelBrick(BrickIdentifiers.repeatLabel, height: .fixed(size: 37), backgroundColor: UIColor.lightGray, dataSource: self)
], inset: 10, edgeInsets: UIEdgeInsets(top: 5, left: 5, bottom: 5, right: 5))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
} else { | ||
data.insert(newElement, at: index) | ||
} | ||
updateRepeatCounts(fixedInsertedIndexPaths: [IndexPath(item: index + 1, section: 1)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BrickKit is designed so the consumer doesn't need to know about indexPaths, so this concerns me a bit. I probably want the API to be a bit more in the trend of
insertItem(for identifier: String, atIndex index: Int)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this
@@ -12,7 +12,9 @@ import BrickKit | |||
private let Section = "Section" | |||
|
|||
class InsertBrickViewController: BrickApp.BaseBrickController, HasTitle { | |||
|
|||
|
|||
var data: [String] = ["Brick 1"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repeated bricks have the title let newElement = "BRICK \(data.count + 1)"
. So this should be upper case String
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
var titles: [String] { | ||
return ["No animation", "From top", "From bottom"] | ||
var appearTitles: [String] { | ||
return ["No animation", "From top", "From bottom", "Fade in"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look good on an iPhone 5(S). Can we abbreviate these a bit more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
de070cc
to
fc7d529
Compare
…ctionView and the collectionView will be responsive to the change.
fc7d529
to
88d16ee
Compare
Items can now be inserted and deleted from anywhere in the brickCollectionView and the collectionView will be responsive to the change.