Initial implementation #1
Conversation
| <string> | ||
| // The MIT License (MIT) | ||
| // | ||
| // Copyright (c) 2015-present Badoo Trading Limited. |
There was a problem hiding this comment.
Copyright date needs to be fixed
| // MARK: - Utility extensions | ||
|
|
||
| private extension String { | ||
| static let cell = "cell" |
|
|
||
| func setItem(_ item: Item) { | ||
| itemId = item.identifier | ||
| textLabel!.text = item.title |
There was a problem hiding this comment.
It's always gonna be there, right?
|
|
||
| override func prepareForReuse() { | ||
| super.prepareForReuse() | ||
| detailTextLabel!.text = nil |
There was a problem hiding this comment.
If you change type in setItem to optional, then you can clean a view state with setItem(nil).
There was a problem hiding this comment.
Actually I don't think I need prepare for reuse here at all. if I replace
if let subtitle = item.subtitle {
detailTextLabel!.text = subtitle
}with
detailTextLabel!.text = item.subtitle|
|
||
| func filter(text: String) { | ||
| searchText = text | ||
| updateFilteredItems() |
There was a problem hiding this comment.
It can be called in searchText.didSet
| switch preferredPresentationStyle { | ||
| case .push: | ||
| guard let navigationController = viewController.navigationController else { | ||
| assertionFailure() |
There was a problem hiding this comment.
It would be nice to have a descriptive message here
|
|
||
| private let store: ItemStore | ||
| private let storage: FavoritesStorageProtocol | ||
| private var favorited: Set<ItemIdentifier> { |
There was a problem hiding this comment.
Why "favorited" here? Everywhere else it's "favorites"
There was a problem hiding this comment.
Needs to be fixed, thanks
| hashValue = ItemIdentifier.hash(id: id.itemId, parents: parents) | ||
| } | ||
|
|
||
| private static func hash(id: String, parents: [String]) -> Int { |
There was a problem hiding this comment.
Consider to use a new Swift syntax for hash function in Swift 4.2
There was a problem hiding this comment.
But it's not available yet right?
|
|
||
| // MARK: - Layout | ||
|
|
||
| extension UICollectionViewLayout { |
There was a problem hiding this comment.
Not sure about file naming. UICollectionViewFlowLayout+Factory - would be more intuitive for me to find this method.
| override func viewDidLoad() { | ||
| super.viewDidLoad() | ||
|
|
||
| collectionView = UICollectionView(frame: .zero, |
There was a problem hiding this comment.
Let's initiate it init as the rest of the views - dropping ! as well from declaration.
|
|
||
| override func prepareForReuse() { | ||
| super.prepareForReuse() | ||
| detailTextLabel!.text = nil |
There was a problem hiding this comment.
Let's clean textLabel as well.
Maybe we should reset isFavourite to default state - if there is such.
There was a problem hiding this comment.
Actually I've already removed prepareForReuse code. I think it's not require to clean it here, because I update all the fields every time I set an item
| override func viewDidLoad() { | ||
| super.viewDidLoad() | ||
|
|
||
| tableView = UITableView(frame: .zero, style: .grouped) |
| tableView.alignAllEdgesWithSuperview() | ||
|
|
||
| if #available(iOS 11, *) { | ||
| addSearch() |
There was a problem hiding this comment.
Are there any fallback or is it only for iOS 11 exclusively?
There was a problem hiding this comment.
I don't think it's worth it
| THE SOFTWARE. | ||
| */ | ||
|
|
||
| public protocol ElementsProviding: TestCaseNameProviding { |
There was a problem hiding this comment.
Would it be reasonable to compose those protocols at client side (ElementsProviding & TestCaseNameProviding) instead of inheriting?
|
|
||
| protocol FavoritesStorageProtocol { | ||
| func save(favorites: [ItemIdentifier]) | ||
| func load() -> [ItemIdentifier]? |
There was a problem hiding this comment.
can we avoid optionality here and simply return [] when there's no favorites?
No description provided.