Xcode 8 + Swift 3 #24
Conversation
mohpor
force-pushed
the
Swift3-Release
branch
from
35dfc4c to
da1f0a9
Compare
|
Some notes: I have tested the framework locally on my computer and again on my devices, ran the test and everything went fine, all tests' passed. Except, when actually testing it on device, the TwitterBot example would crash upon displaying the action sheet, complaining about internal inconsistency with number of rows and the data set. It's worth taking a look. |
mohpor
force-pushed
the
Swift3-Release
branch
from
da1f0a9 to
43b6f93
Compare
There was a problem hiding this comment.
@mohpor thanks for updating your PR. I just finished reviewing it and I left some minor comments.
I think that the crash in the Tweetbot example is caused by a wrong value returned in the function collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int. Check out my comment about it for more datils
| if hasHeader() && section == 0 { return 0 } | ||
| if settings.behavior.useDynamics && section > _dynamicSectionIndex { | ||
| if settings.behavior.useDynamics && section > _dynamicSectionIndex ?? 0 { |
There was a problem hiding this comment.
This is not exactly the same, now if _dynamicSectionIndex is null and section is 0, then this function si returning _sections[actionSectionIndexFor(section)].actions.count. While previously it returned 0.
Additionally, this is causing the crash that you saw. When _dynamicSectionIndex is null, it means that no data has been added to the collection view (in the case useDynamics==true). So the collection view datasource is returning x number of rows for section 0 when no one has been inserted.
Next code should be fix the crash:
open func collectionView(_ collectionView: UICollectionView, numberOfItemsInSection section: Int) -> Int {
if hasHeader() && section == 0 { return 0 }
let rows = _sections[actionSectionIndexFor(section)].actions.count
guard let dynamicSectionIndex = _dynamicSectionIndex else {
return settings.behavior.useDynamics ? 0 : rows
}
if settings.behavior.useDynamics && section > dynamicSectionIndex {
return 0
}
return rows
}| let action = self.actionForIndexPath(actionIndexPathFor(indexPath)) | ||
|
|
||
| if let action = action where action.executeImmediatelyOnTouch { | ||
| if let action = action , action.executeImmediatelyOnTouch { |
There was a problem hiding this comment.
please remove the white space before the comma
| cell.alpha = action.enabled ? 1.0 : 0.5 | ||
| } | ||
| } | ||
|
|
||
| required public init?(coder aDecoder: NSCoder) { |
There was a problem hiding this comment.
please indent using 4 spaces
|
|
||
| onConfigureCellForAction = { cell, action, indexPath in | ||
| cell.setup(action.data?.title, detail: action.data?.subtitle, image: action.data?.image) | ||
| cell.alpha = action.enabled ? 1.0 : 0.5 | ||
|
|
||
| UIView.animateWithDuration(0.30) { | ||
| UIView.animate(withDuration: 0.30) { |
There was a problem hiding this comment.
Sure, But I fail to see the point of an empty animation block! Did you mean to put that alpha value change inside the animation block?
| } | ||
| } | ||
|
|
||
| view.addSubview(collectionView) | ||
|
|
||
| // calculate content Inset | ||
| collectionView.layoutSubviews() | ||
| if let section = _sections.last where !settings.behavior.useDynamics { | ||
| if let section = _sections.last , !settings.behavior.useDynamics { |
There was a problem hiding this comment.
please delete white space before comma
There was a problem hiding this comment.
This is caused by the Xcode migrator tool which replaced where with , (Notice that there is a whitespace before and after where but there shouldn't be white space there. I shouldn't have overlooked these. Sorry)
| super.viewDidLayoutSubviews() | ||
| collectionView.collectionViewLayout.invalidateLayout() | ||
| } | ||
|
|
||
| public override func prefersStatusBarHidden() -> Bool { | ||
| open override var prefersStatusBarHidden : Bool { |
There was a problem hiding this comment.
please delete white space before colon
| return !settings.statusBar.showStatusBar | ||
| } | ||
|
|
||
| public override func preferredStatusBarStyle() -> UIStatusBarStyle { | ||
| open override var preferredStatusBarStyle : UIStatusBarStyle { |
There was a problem hiding this comment.
here too and please remove extra space between open and override
| action.handler?(action) | ||
| } | ||
|
|
||
| self.dismiss() { | ||
| if let action = action where !action.executeImmediatelyOnTouch { | ||
| if let action = action , !action.executeImmediatelyOnTouch { |
There was a problem hiding this comment.
please remove space before comma
|
Just noticed this PR and was wondering if it will get merged in at some point? This is the relevant issue for this #25 |
|
Looking forward to the merge, great work! |
|
👍 |
mohpor
force-pushed
the
Swift3-Release
branch
from
43b6f93 to
ee5d161
Compare
|
@m-revetria, I have rebased the PR based on your comments. |
|
@mohpor great work! thanks for your help and your time updating this PR! |
No description provided.