Map feature #40
Map feature #40
Conversation
… opening map on mobile and MapBox is the map container)
…as a type created an error
| interface IMarkerProps { | ||
| latitude: number; | ||
| longitude: number; | ||
| showPopUp: () => void; |
There was a problem hiding this comment.
Can you change this prop name to onClick as it reflects that action that has to be performed?
| <div> | ||
| <p className="w-40 font-extrabold">{property?.title}</p> | ||
| <p className="overflow-hidden break-words w-40 text-xs"> | ||
| {property?.description} | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
You don't need to wrap the children inside the div as you have a parent component Popup.
You can remove that div or just make use of React Fragment.
| const [lng] = useState(32.58252); | ||
| const [lat] = useState(0.347596); |
There was a problem hiding this comment.
It's better to merge the two in one state.
| const showPopUp = () => { | ||
| setDisplayPopUp(true); | ||
| }; | ||
| const closePopUp = () => { | ||
| setDisplayPopUp(false); | ||
| }; |
There was a problem hiding this comment.
Can you only create one function that toggles the popup?
| const MapButton = ({ onClick }: IProps): ReactElement => { | ||
| const [isMobile] = useResponsive(); | ||
| if (!isMobile) { | ||
| return ( | ||
| <> | ||
| <button | ||
| type="button" | ||
| className="w-[80%] p-1 bg-brand-bold text-sm text-white rounded-lg px-3 py-2 mt-3 " | ||
| onClick={onClick} | ||
| > | ||
| Voir sur map | ||
| </button> | ||
| </> | ||
| ); | ||
| } | ||
| return ( | ||
| <> | ||
| <button | ||
| type="button" | ||
| className="w-full p-3 bg-brand-bold text-white rounded-lg md:mx-auto md:px-16 mt-4 " | ||
| onClick={onClick} | ||
| > | ||
| Voir sur map | ||
| </button> | ||
| </> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
This is nice but you don't need to create a component for this button as there are no extra kinds of stuff that you are handling.
There is also no need of making use of useResponsive hook.
You can handle responsiveness with just tailwind.
| import RelatedProperties from './RelatedProperties'; | ||
| import PropertyCarousel from './PropertyCarousel'; | ||
| import PropertyDetails from './PropertyDetails'; | ||
| import PropertyAgent from './PropertyAgent'; | ||
| import PropertyImages from './PropertyImages'; | ||
| import PropertyDetailsDesktop from './PropertyDetailsDesktop'; | ||
| import MapButton from '../../__modules__/MapButton/index'; |
There was a problem hiding this comment.
You can ignore /index
| const openMapBox = () => { | ||
| setOpenMap(true); | ||
| setShowMapButtom(false); | ||
| }; | ||
|
|
||
| const closeMapBox = () => { | ||
| setOpenMap(false); | ||
| setShowMapButtom(true); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Make use of one toggle function here too.
| MAPBOX_ACCESS_TOKEN: | ||
| 'pk.eyJ1Ijoiam9uYXRoemloaW5kdWxhIiwiYSI6ImNreW4wZXBwbTNsd2EydW4wamJ1aTdidHgifQ.zVkgrvMk02fZiJFaV_jkDQ', | ||
| MAPBOX_CUSTOM_STYLE_URL: | ||
| process.env.MAPBOX_CUSTOM_STYLE_URL || | ||
| 'mapbox://styles/jonathzihindula/ckynaitbt8tyh14qpylblnbwp', |
There was a problem hiding this comment.
Read these keys from .env file.
…e hook, toggle popup handle in one function
|
Thanks for the review ! I made already all the correction suggested in the review. |
| const togglePopUp = () => { | ||
| if (displayPupUp) setDisplayPopUp(false); | ||
| else { | ||
| setDisplayPopUp(true); | ||
| } |
There was a problem hiding this comment.
No need to make use of if statement here.
There was a problem hiding this comment.
can i use switch ? cause i don't understand why the is should be removed !
There was a problem hiding this comment.
Nope. I was meaning I have to use it like the following:
setDisplayPopup(!displayPopup)
Which will be toggling the value of displayPopup
There was a problem hiding this comment.
I mean why the if statement should be removed
There was a problem hiding this comment.
Ahhh okey ! i get it ! thanks
| const toggleMap = () => { | ||
| if (!openMap && showMapButton) { | ||
| setOpenMap(true); | ||
| setShowMapButtom(false); | ||
| } else { | ||
| setOpenMap(false); | ||
| setShowMapButtom(true); | ||
| } |
There was a problem hiding this comment.
No need for the if statement here either.
You can also rename the function to onClick
|
I made already all the correction suggested in the second review. |
I integrate mapBox to get the map view of the property and places around like hospital, markets,etc.
I used Kampala's coordinates a to show the feature's behavior