feat: Fix map #351
feat: Fix map #351
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request transitions the search functionality from a radius-based approach to a dynamic bounding box system that uses the visible map area for location filtering. It also improves the user interface with enhanced result headers showing result ranges and better loading states.
- Replaces fixed-radius search with dynamic bounding box calculated from the visible map area
- Adds improved search results header showing "Showing X-Y of Z results" format with Clear Search button
- Introduces loading states during map initialization to prevent premature queries
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.config.js | Fixes typo in API_URL configuration and adjusts publicPath for development |
| app/utils/useAppContext.tsx | Adds bounding box state management and changes default radius from "all" to 1600 |
| app/pages/SearchResultsPage/SearchResultsPage.tsx | Implements bounding box search logic with map initialization checks |
| app/components/ui/SearchResultsHeader.tsx | Refactors to show result ranges and includes Clear Search button |
| app/components/SearchAndBrowse/SearchMap/SearchMap.tsx | Adds bounding box calculation from map bounds on search actions |
| Various other files | Updates to support new search paradigm with styling and test adjustments |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| { | ||
| context: ["/api-docs"], | ||
| target: config.API_UR || "http://localhost:3000", | ||
| target: config.API_URL || "http://localhost:3000", |
There was a problem hiding this comment.
Fixed typo in variable name from API_UR to API_URL.
rosschapman
left a comment
rosschapman
left a comment
There was a problem hiding this comment.
@chadbrokaw I took a look at this earlier because as I've expressed, I'm excited to watch this codebase develops. Awesome stuff!
There was a problem hiding this comment.
praise: 😌 Really nice comments in handleSearchThisAreaClick() otherwise those operations are pretty opaque not behind meaningful names.
| case SearchMapActions.MapInitialized: | ||
| // Map has initialized and bounding box is now available | ||
| setIsMapInitialized(true); | ||
| return; |
There was a problem hiding this comment.
question: How do you find this little action dispatch mechanism? I was inspired by Redux-like stores. I had hoped it would be utilized further.
| inSanFrancisco: false, | ||
| }, | ||
| aroundUserLocationRadius: "all", | ||
| aroundUserLocationRadius: 1600, |
There was a problem hiding this comment.
suggestion (if-minor): What about putting this magic number in utils/location.tsx with DEFAULT_AROUND_PRECISION?
| output: { | ||
| path: buildDir, | ||
| publicPath: "/dist/", | ||
| publicPath: process.env.NODE_ENV === "development" ? "/" : "/dist/", |
There was a problem hiding this comment.
question (if-minor): Why this change out of pure curiosity?
| if (map) { | ||
| // Get the visible bounds of the map | ||
| const bounds = map.getBounds(); | ||
| if (bounds) { |
There was a problem hiding this comment.
question (if-minor): Out of pure curiosity, did you find that bounds might be falsy sometimes?
| // Set initial bounding box when map is first loaded | ||
| const idleListener = map.addListener("idle", () => { | ||
| // Remove the listener so it only fires once | ||
| google.maps.event.removeListener(idleListener); |
There was a problem hiding this comment.
thought: Man this kind of variable access in JS (via "hoisting") always breaks my brain. What about expanding this comment so folks unfamiliar with gmaps might understand this a little better:
Remove the listener so it only fires once on initialization because the `idle` event will continue to fire when the map finishes panning, zooming, or loading.
📝 Description
This pull request introduces a significant update to how location-based search results are retrieved and displayed, transitioning from a radius-based search to a dynamic bounding box approach that leverages the visible map area. It also improves the user experience with enhanced result headers and loading states, and refines code structure for better maintainability. The most important changes are grouped below:
Search Logic and Map Integration:
app/components/SearchAndBrowse/SearchMap/SearchMap.tsx,app/pages/SearchResultsPage/SearchResultsPage.tsx,app/utils/useAppContext.tsx) [1] [2] [3]app/pages/SearchResultsPage/SearchResultsPage.tsx)User Interface Enhancements:
SearchResultsHeadercomponent and integrated across search and browse result pages. (app/components/ui/SearchResultsHeader.tsx,app/components/SearchAndBrowse/SearchResults/SearchResults.tsx,app/pages/BrowseResultsPage/BrowseResultsPage.tsx,app/pages/SearchResultsPage/SearchResultsPage.tsx) [1] [2] [3] [4]app/pages/SearchResultsPage/SearchResultsPage.module.scss,app/pages/SearchResultsPage/SearchResultsPage.tsx) [1] [2]Styling Adjustments:
app/components/SearchAndBrowse/SearchResults/SearchResults.module.scss) [1] [2]Testing Improvements:
app/pages/SearchResultsPage/SearchResultsPage.test.tsx) [1] [2]Context and State Management:
useAppContext) is extended to handle the bounding box and its updater, and the default search radius is set to 1600 meters instead of "all". (app/utils/useAppContext.tsx,app/App.tsx,app/App.test.tsx) [1] [2] [3]These changes collectively modernize the search experience, making it more intuitive and responsive to user interactions with the map.