Display map #641
Display map #641
Conversation
| import { media } from "../../utils/media"; | ||
|
|
||
| interface SuggestionProps { | ||
| $isHighlighted?: boolean; |
There was a problem hiding this comment.
Could you please tell us why there is$ at the beginning?
There was a problem hiding this comment.
https://styled-components.com/docs/api#primary
"Transient props
If you want to prevent props meant to be consumed by styled components from being passed to the underlying React node or rendered to the DOM element, you can prefix the prop name with a dollar sign ($), turning it into a transient prop."
app/javascript/react/components/LocationSearch/LocationSearch.style.tsx
Outdated
Show resolved
Hide resolved
| }); | ||
|
|
||
| useEffect(() => { | ||
| status === "OK" && data.length && setItems(data); |
There was a problem hiding this comment.
can we use some constant when it comes to status possible states and then use here ex. Status.OK?
There was a problem hiding this comment.
This is wip - I will not dig into this code right now. This LocationSearch is from Kasia POC and it will done in different task.
| </S.LocationSearchButton> | ||
| )} | ||
| </S.SearchContainer> | ||
| {/* <S.SuggestionsList {...getMenuProps()}> |
There was a problem hiding this comment.
should be left in here?
There was a problem hiding this comment.
Same answer as the previous one
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| box-shadow: 2px 2px 4px 0px rgba(76, 86, 96, 0.1); |
There was a problem hiding this comment.
color for shadow from colors struct please, please skip if not possible to change
| setLocation={setLocation} | ||
| mapRef={mapRef as React.RefObject<Map>} | ||
| setNavMenuVisible={setNavMenuVisible} | ||
| t={t} |
There was a problem hiding this comment.
what is this magic t 🧞♂️
There was a problem hiding this comment.
Here I am just passing translation as a props 😄
| lng: number; | ||
| }; | ||
|
|
||
| const trees: RawTree[] = [ |
There was a problem hiding this comment.
I understand this is only for the development.
Let's add a TODO in the code and a cleanup ticket. WDYT?
There was a problem hiding this comment.
I will add TODO and add comment in existing ticket which is connected to Markers and Clusters
| return ( | ||
| <APIProvider | ||
| apiKey={googleMapsApiKey} | ||
| onLoad={() => console.log("Maps API has loaded.")} |
There was a problem hiding this comment.
Do we need this console.log on prod or is it only for development purposes?
There was a problem hiding this comment.
It is only for development purposes
| import { MarkerClusterer } from "@googlemaps/markerclusterer"; | ||
| import type { Marker } from "@googlemaps/markerclusterer"; | ||
|
|
||
| type Point = google.maps.LatLngLiteral & { key: string }; |
There was a problem hiding this comment.
WDYT about reusing LatLngLiteral type from app/javascript/react/types/googleMaps.ts?
|
|
||
| type AutocompletePrediction = google.maps.places.AutocompletePrediction; | ||
|
|
||
| const LocationSearch = ({ setLocation, isMapPage }: LocationSearchProps) => { |
There was a problem hiding this comment.
I see that we're passing isMapPage depending on the context Mobile/Desktop header. Is it connected with the map page or rather a mobile view? 🤔
| const { t } = useTranslation(); | ||
|
|
||
| const isMapPage = window.location.pathname === "/"; |
There was a problem hiding this comment.
This approach carries a risk since any URL change can disrupt this functionality. It might be worth reconsidering the design of the navbar. Instead of trying to adapt a single navbar for various views (mobile, desktop, map, non-map), we could explore the possibility of having separate navbars for each page. This isn't necessarily something to address in this PR, but rather a point to consider for future improvements.
There was a problem hiding this comment.
I would love to use different approach but at this point it was the easiest solution i could think of. We can discuss it about but I would not change in this PR. For now i can just put this url to const
| background-color: transparent; | ||
| z-index: 2; | ||
| flex-wrap: wrap; | ||
| position: absolute; |
There was a problem hiding this comment.
Are we confident that this change won't impact the current AirCasting page? I'm just double-checking since I'm not fully familiar with the feature flag setup. Is the current page built entirely in Elm, meaning there are no concerns to worry about?
There was a problem hiding this comment.
I think it is ok. React App is not connected yet with the production version
app/javascript/react/components/Map/ControlPanel/ControlPanel.tsx
Outdated
Show resolved
Hide resolved
Thank you. When a switch or checkbox is not complicated, I prefer to build it on my own instead of using a library. In this case, I can style it and rebuild it however I want. |
In this PR:
Trello card