BER-80: Converting MapMarkerDetailView into SwiftUI #341
Conversation
Chhumbucket
changed the title
Chhumbucket
changed the title
… if I have the right amount of newlines
…se need transition will add
Screen.Recording.2025-03-28.at.11.04.52.PM.mov |
Chhumbucket
dismissed stale reviews from justinwongeecs and baeuke
Fixed the conflicts
With the smaller text, I remove empty space
|
@Chhumbucket I think it was good with a little bit of padding on top of the title. Also, can you align title with the description text?cuz the title is offset to the left compared to description |
Fixed the alignment now |
why does the description length now extend until the edge |
When I removed the horizontal padding it removed the padding in the back now it should be fixed |
|
Current build: Also, I think the cards should be more wider? It does look a bit scrunched. I think there are other areas of improvement. The icons at the bottom I feel are bit too large. Try to compare what we have currently and see if you can emulate it. |
How do we feel abt this UI? @justinwongeecs |
|
@Chhumbucket Maybe choose a padding that is a bit less than the right side? It feels that there's a bit too much negative space on the right. |
|
Search is breaking in your branch: 
|
| searchResultsView = UIHostingController(rootView: SearchResultsView().environmentObject(searchViewModel)).view | ||
| let resultsView = SearchResultsView().environmentObject(searchViewModel) | ||
| searchResultsHostingController = UIHostingController(rootView: AnyView(resultsView)) | ||
| searchResultsView = searchResultsHostingController.view |
There was a problem hiding this comment.
@Chhumbucket You didn't resolve this one. I think we had a consistent way with the hostingController;
you better make markerDetail work same way too than changing how search components are made (don't change search).
Justin did in the same way with mapUserLocationButton.
| @@ -50,19 +49,18 @@ class MapViewController: UIViewController, SearchDrawerViewDelegate { | |||
| private var searchBarViewController: UIViewController! | |||
| private var searchBar: UIView! | |||
| private var searchResultsView: UIView! | |||
| private var searchResultsHostingController: UIHostingController<AnyView>! | |||
| private var searchViewModel: SearchViewModel! | |||
There was a problem hiding this comment.
We don't need to store UIHostingController variables. The reason previously we added searchBarViewController (which is a hosting controller) is because we needed to add it as a child to mapViewController to share environment and focus state (which was specific to how search works and the whole setup here). in this case there is no need for that.
| @@ -71,14 +69,13 @@ class MapViewController: UIViewController, SearchDrawerViewDelegate { | |||
| private let mapUserLocationViewModel = MapUserLocationButtonViewModel() | |||
| private let mapMarkersDropdownViewModel = MapMarkersDropdownViewModel() | |||
| private var mapMarkers: [[MapMarker]] = [] | |||
| private var markerDetail: MapMarkerDetailView! | |||
| private var markerDetail: UIHostingController<MapMarkerDetailSwiftView>! | |||
|
|
|||
There was a problem hiding this comment.
Same here. You don't need markerDetail: UIHostingController<MapMarkerDetailSwiftView>!
just make markerDetail: UIView, and follow same setup process as for mapUserLocationBtn, mapMarkersDropdown, and Search components
|
|
||
| fetchFromMapDataSource() | ||
| createMapLocationButton() | ||
| createMapMarkerDropdownButton() | ||
|
|
||
| self.view.addSubViews([mapView, mapUserLocationButton, mapMarkersDropdownButton, markerDetail, searchResultsView, searchBar]) | ||
| self.view.addSubViews([mapView, mapUserLocationButton, mapMarkersDropdownButton, markerDetail.view, searchResultsView, searchBar]) | ||
| setupSubviews() |
There was a problem hiding this comment.
when consistent, here you add just markerDetail, not markerDetail.view
| import MapKit | ||
| import SwiftUI | ||
|
|
||
| // MARK: - MapMarkerDetailSwiftView |
| var onClose: (() -> Void)? | ||
| var body: some View { |
There was a problem hiding this comment.
Add newline between onClose and body
| var marker: MapMarker? | ||
| var onClose: (() -> Void)? | ||
| var body: some View { | ||
| ZStack { |
There was a problem hiding this comment.
Is this ZStack necessary?
| let markerColor: Color = { | ||
| guard let marker else { | ||
| return .purple | ||
| } | ||
|
|
||
| switch marker.type { | ||
| case .known(let type): | ||
| return Color(type.color()) | ||
| case .unknown: | ||
| return Color(BMColor.MapMarker.other) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Maybe make this is a private helper function?
| Button { | ||
| onClose?() | ||
| } label: { | ||
| Image(systemName: "xmark") | ||
| .font(.system(size: 16)) | ||
| .foregroundStyle(Color.secondary) | ||
| .padding(.trailing, 4) | ||
| } |
There was a problem hiding this comment.
Use trailing closure syntax:
Button(action: {
}) {
...
}| HStack { | ||
| HStack(spacing: 8) { | ||
| Image(systemName: "clock") | ||
| .font(.system(size: 12)) | ||
| .foregroundColor(.secondary) | ||
| .rotationEffect(.init(degrees: 90)) | ||
| openStatusButton | ||
| } | ||
|
|
||
| Spacer() | ||
|
|
||
| locationInfoView | ||
|
|
||
| Spacer() | ||
|
|
||
| categoryView | ||
| } |
There was a problem hiding this comment.
Is the outer HStack necessary? Could we put lines 100-106 within the nested HStack?
|
|
||
| private var openStatusButton: some View { | ||
| Capsule() | ||
| .fill(marker?.isOpen ?? false ? Color.blue : Color(red: 0.4, green: 0.5, blue: 0.9)) |
There was a problem hiding this comment.
Color.blue → .blue?
| Group { | ||
| if let marker, case .known(let type) = marker.type, type == .cafe, let mealPrice = marker.mealPrice { | ||
| Text(mealPrice) | ||
| .font(Font(BMFont.regular(12))) | ||
| .foregroundColor(.primary) | ||
| } else { | ||
| Text("<10") | ||
| .font(Font(BMFont.regular(12))) | ||
| .foregroundColor(.primary) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bring .font(Font(BMFont.regular(12))) and .foregroundColor(.primary) out and apply it to Group?
| private func getCategoryIcon() -> String { | ||
| guard let marker else { | ||
| return "questionmark.circle" | ||
| } | ||
|
|
||
| switch marker.type { | ||
| case .known(let type): | ||
| switch type { | ||
| case .cafe: | ||
| return "fork.knife" | ||
| case .store: | ||
| return "bag" | ||
| case .mentalHealth: | ||
| return "brain" | ||
| case .genderInclusiveRestrooms: | ||
| return "toilet" | ||
| case .menstrualProductDispensers: | ||
| return "drop" | ||
| case .garden: | ||
| return "leaf" | ||
| case .bikes: | ||
| return "bicycle" | ||
| case .lactation: | ||
| return "heart" | ||
| case .rest: | ||
| return "bed.double" | ||
| case .microwave: | ||
| return "bolt" | ||
| case .printer: | ||
| return "printer" | ||
| case .water: | ||
| return "drop.fill" | ||
| case .waste: | ||
| return "trash" | ||
| case .none: | ||
| return "mappin" | ||
| } | ||
| case .unknown: | ||
| return "mappin" | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't we have a icon() method on a MapMarker that returns a UIImage? maybe use that?
Converted the MapMarkerDetailView into SwiftUI.
Currently placed templates for the buttons and used static data just to represent the view.
Future updates,
I want to connect the title of the place and the description in the same place to make sure that the padding is aligned. I feel that there is redundancy in the code, I want some feedback if there is.