Add collection view for menu view #17
Conversation
|
@AAAstorga any idea why bitrise errors on the review? |
|
|
||
| var titleLabel: UILabel! | ||
| var ingredientsLabel: UILabel! | ||
| var detailStackView: UIStackView! //include title and ingredients |
| selectButton = UIButton() | ||
| selectButton.layer.cornerRadius = 8 | ||
| selectButton.setTitle("Select", for: .normal) | ||
| selectButton.backgroundColor = UIColor(red: 1.0, green: 156.0 / 255.0, blue: 29.0 / 255.0, alpha: 1.0) |
There was a problem hiding this comment.
this should be defined in UIColor, and not here
| let selectButtonWidth: CGFloat = 84 | ||
| let imageViewWidthHeight: CGFloat = 64 | ||
| let imageViewStackViewHorizontalSpacing: CGFloat = 24 | ||
| let stackViewSelectButtonHorizontalSpacing: CGFloat = 17 |
There was a problem hiding this comment.
@kevinchan159 should this code be here or at the top of the file?
There was a problem hiding this comment.
No, we're changing constants conventions to keep them at the top of the function. @young-k
| } | ||
|
|
||
| // MARK: Constraint setup | ||
| private func setUpConstraints() { |
There was a problem hiding this comment.
should be setupConstraints, not setUpConstraints
There was a problem hiding this comment.
When this discussion happened last year on Recast we decided to go with setUp because it's in the verb form of "setting up" constraints -- are we changing this? @drewsdunne
| detailStackView.snp.makeConstraints { make in | ||
| make.leading.equalTo(imageView.snp.trailing).offset(imageViewStackViewHorizontalSpacing) | ||
| make.centerY.equalToSuperview() | ||
| make.trailing.equalTo(selectButton.snp.leading).offset(-stackViewSelectButtonHorizontalSpacing) |
There was a problem hiding this comment.
centerY should be before leading, it's alphabetical and more logical
| var detailStackView: UIStackView! //include title and ingredients | ||
|
|
||
| var additionalPricingLabel: UILabel! | ||
| var selectButton: UIButton! |
There was a problem hiding this comment.
prefer that these variables are alphabetized or grouped by type, up to @drewsdunne or @kevinchan159
| // MARK: Lifecycle | ||
| override init(frame: CGRect) { | ||
| super.init(frame: frame) | ||
|
|
There was a problem hiding this comment.
does a newline here add clarity to the code? if not, it can be removed!
| } | ||
|
|
||
| // MARK: Constraint setup | ||
| func setUpConstraints() { |
There was a problem hiding this comment.
shouldn't this be private? you put it up top, but not here
|
@young-k bitrise is not setup for this one yet. I'm working on it |
| contentView.addSubview(detailStackView) | ||
|
|
||
| detailStackView.addArrangedSubview(titleLabel) | ||
| detailStackView.addArrangedSubview(ingredientsLabel) |
There was a problem hiding this comment.
you can pass this in through the init UIStackView(arrangedSubviews: [...])
|
|
||
| titleLabel = UILabel() | ||
| titleLabel.font = UIFont.systemFont(ofSize: 17, weight: .bold) | ||
| titleLabel.textColor = .black |
There was a problem hiding this comment.
any reason why we aren't adding to contentView
| } | ||
|
|
||
| // MARK: Constraint setup | ||
| func setUpConstraints() { |
There was a problem hiding this comment.
add private and change to setupConstraints
| // MARK: Constraint setup | ||
| func setUpConstraints() { | ||
| collectionView.snp.makeConstraints { make in | ||
| make.edges.equalToSuperview() |
There was a problem hiding this comment.
i don't think make.edges pins the top and bottom to the safeAreaLayoutGuide
| currentMenuItem = object | ||
| } else { | ||
| return | ||
| } |
There was a problem hiding this comment.
you can simplify to currentMenuItem = object as? MenuItem
| class MenuViewController: UIViewController { | ||
|
|
||
| var collectionView: UICollectionView! | ||
| var adapter: ListAdapter! |
There was a problem hiding this comment.
Could we call this listAdapter to be consistent with our other view controllers.
Resolves #8