No issue: map view updates with places #546
Conversation
mcomella
force-pushed
the
ni-map-view
branch
from
05ce60d
to
a7cff7c
Compare
| get { return layer.shadowRadius } | ||
| set { layer.shadowRadius = newValue } | ||
| } | ||
| public var shadowOffset: CGSize { |
There was a problem hiding this comment.
Any specific reason you're exposing these layer properties as properties on the view? If I were using this class, I would expect to do expandingCardView.layer.shadowColor = ... like any other UIView.
|
|
||
| private lazy var scrollView: UIScrollView = { | ||
| let view = ObservableScrollView() | ||
| view.onContentSizeUpdated = { [unowned self] contentSize in |
There was a problem hiding this comment.
Note that this is typically done use the delegate pattern (ObservableScrollViewDelegate with didUpdateContentSize, for example). That way, the client code isn't responsible for breaking the ref cycle, which is error prone.
| import SnapKit | ||
|
|
||
| private let cardPadding = 10 | ||
| private let cardSpacing: CGFloat = 10 |
There was a problem hiding this comment.
Should these go into the Style struct you made?
There was a problem hiding this comment.
Global constants/strings/etc. create a lot of bloat in files until it's hard to maintain, easy to loose track of unused values, and difficult to identify what is safe to re-use (and what just happens to be in a giant constants file). I prefer to keep the constants locally to the places they're used. A good example is our FFAndroid styles which are so hard to maintain (most of these styles should really be embedded directly in the layout files). fwiw, I'm conflicted about our Colors.swift & Fonts.swift files.
Style.swift was intended to be for shared styles – should I rename it as such?
| private let cardSpacing = 18 | ||
| private let cardProviderSpacing = 12 | ||
|
|
||
| private let cardCoverPhotoHeight = 72 |
There was a problem hiding this comment.
Same question here re: Style.
|
|
||
| private lazy var reviewCountView: UILabel = { | ||
| let view = UILabel() | ||
| view.text = "10 reviews" |
There was a problem hiding this comment.
Re-used our "no info" text.
| @@ -43,8 +43,8 @@ class MapViewCardFooter: ExpandingCardView { | |||
| return view | |||
| }() | |||
|
|
|||
| private lazy var yelpProviderView: UIView = MapViewReviewProvider() | |||
| private lazy var tripAdvisorProviderView: UIView = MapViewReviewProvider() | |||
| private lazy var yelpProviderView: UIView = MapViewYelpReviewProviderView() | |||
I converted these from SVG.
There was a bug where adding a margin to a StackView subview will add a margin to multiple subviews. :|
mcomella
force-pushed
the
ni-map-view
branch
from
a7cff7c
to
6ee4b50
Compare
Still TODO:
Hide card until place is selected(followup Slide card up from bottom when marker first selected #548)How to handle map when place is selected? (return false from delegate?)