Map Responsiveness and Pop -UP #15
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the map responsiveness by adding Airbnb-like anchored popups for spaces and properties, along with viewport-based filtering. The changes improve the user experience by showing interactive popups on desktop while maintaining mobile-friendly bottom sheets, and implementing dynamic content filtering based on the visible map region.
Key Changes
- Added anchored popup components for spaces and properties with MapOverlayPortal system
- Implemented viewport-based filtering to show only spaces visible in current map bounds
- Enhanced mobile responsiveness with conditional popup behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| frontend/src/pages/browse/components/GoogleMap.tsx | Adds MapOverlayPortal, SpaceAnchoredPopup, PropertyAnchoredPopup components and enhanced marker interaction logic |
| frontend/src/pages/browse/BrowsePage.jsx | Implements viewport-based filtering, fallback logic for empty regions, and integrates new map features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| useEffect(() => { | ||
| return () => { | ||
| propertyMarkersRef.current = cleanupMarkers(propertyMarkersRef.current); | ||
| areaMarkersRef.current = cleanupMarkers(areaMarkersRef.current); |
There was a problem hiding this comment.
Missing space after 'cleanupMarkers(areaMarkersRef.current);' - the line should be properly indented to match the surrounding code structure.
| areaMarkersRef.current = cleanupMarkers(areaMarkersRef.current); | |
| areaMarkersRef.current = cleanupMarkers(areaMarkersRef.current); |
| // ✅ CLEAN: Simple marker design with Deep Teal | ||
| const markerElement = document.createElement('div'); | ||
| markerElement.className = 'flex items-center justify-center w-8 h-8 bg-teal-500 text-white rounded-full border-2 border-white shadow-soft cursor-pointer hover:bg-teal-600 transition-all duration-200 hover:scale-110'; | ||
| markerElement.className = 'flex items-center justify-center w-8 h-8 bg-[#4668AB] text-white rounded-full border-2 border-white shadow-soft cursor-pointer hover:bg-[#3c5997] transition-all duration-200 hover:scale-110'; |
There was a problem hiding this comment.
The color values '#4668AB' and '#3c5997' are hardcoded magic values that appear multiple times throughout the file. Consider extracting these into named constants or CSS variables for better maintainability.
| const price = space.baseRate | ||
| ? `$${space.baseRate}${space.rateType ? '/' + space.rateType.toLowerCase().replace('ly','').replace('y','y') : ''}` |
There was a problem hiding this comment.
The string manipulation logic .replace('ly','').replace('y','y') is flawed - it removes 'ly' but then adds 'y' back, which would result in incorrect transformations. For example, 'yearly' would become 'yeary' instead of 'year'.
| const price = space.baseRate | |
| ? `$${space.baseRate}${space.rateType ? '/' + space.rateType.toLowerCase().replace('ly','').replace('y','y') : ''}` | |
| // Map rateType to singular form for display | |
| const rateTypeMap: { [key: string]: string } = { | |
| daily: 'day', | |
| weekly: 'week', | |
| monthly: 'month', | |
| yearly: 'year', | |
| }; | |
| const price = space.baseRate | |
| ? `$${space.baseRate}${space.rateType ? '/' + (rateTypeMap[space.rateType.toLowerCase()] || space.rateType.toLowerCase()) : ''}` |
| const lngOk = east >= west | ||
| ? c.lng <= east && c.lng >= west | ||
| : (c.lng <= east || c.lng >= west); // antimeridian wraparound |
There was a problem hiding this comment.
The antimeridian wraparound logic is incorrect. When east < west (crossing 180°/-180°), a longitude is valid if it's either >= west OR <= east, but the current condition uses AND which would never be true when crossing the antimeridian.
|
|
||
| mapInstanceRef.current = map; | ||
|
|
||
| // No need for a persistent projection overlay; MapOverlayPortal manages its own |
There was a problem hiding this comment.
This comment appears to be incomplete or refers to removed code. It should either be removed or expanded to clearly explain what MapOverlayPortal manages and why no persistent overlay is needed.
| // No need for a persistent projection overlay; MapOverlayPortal manages its own | |
| // No need for a persistent projection overlay here. | |
| // MapOverlayPortal is responsible for rendering overlays that require map projection. | |
| // It creates and manages its own overlay instances as needed, so a persistent overlay is unnecessary in this component. |
No description provided.