-
-
Notifications
You must be signed in to change notification settings - Fork 173
feat: add search and select functionality for crags #1184 #1383
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Fix a sleepy mistake
fix: bugs fixed after review
mikeschen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments. I will try to test out sometime soonish, I am slammed with day job and contracting job which is why I havent reviewed sooner :(
| if (!mapInstance) return | ||
| const features = mapInstance.querySourceFeatures('crags', { sourceLayer: 'crags' }) | ||
| const cragsFeatures = features | ||
| .map(f => tileToFeature('crag-name-labels', {x:0,y:0}, f.geometry, f.properties as TileProps, mapInstance)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use var name as "feature" rather than "f"
| } | ||
|
|
||
| const filtered = cragsList.filter(c => | ||
| c.data.areaName.toLowerCase().includes(value.toLowerCase()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use "crag" rather than "c" for var name
| /> | ||
| {suggestions.length > 0 && ( | ||
| <ul className='absolute top-full left-0 w-full bg-white shadow-lg rounded-md max-h-48 overflow-y-auto z-50'> | ||
| {suggestions.map(c => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use "suggestion"rather than "c" for var name
| const features = mapInstance.querySourceFeatures('crags', { sourceLayer: 'crags' }) | ||
| const cragsFeatures = features | ||
| .map(f => tileToFeature('crag-name-labels', {x:0,y:0}, f.geometry, f.properties as TileProps, mapInstance)) | ||
| .filter((f): f is ActiveFeature => f !== null) // фильтруем null и сохраняем тип |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds search and selection functionality for crags in the map toolbar, allowing users to quickly find and navigate to specific climbing areas.
Key Changes:
- Added search input with live filtering of crags by name
- Implemented crag selection that highlights the selected crag and flies the map to its location
- Extracted all crags from map features into state for search functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/components/maps/MapToolbar.tsx | Added search input, suggestions dropdown, and handlers for filtering and selecting crags |
| src/components/maps/GlobalMap.tsx | Added state for all crags, extraction logic from map features, and highlight/fly-to function for selected crags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const features = mapInstance.querySourceFeatures('crags', { sourceLayer: 'crags' }) | ||
| const cragsFeatures = features | ||
| .map(f => tileToFeature('crag-name-labels', {x:0,y:0}, f.geometry, f.properties as TileProps, mapInstance)) | ||
| .filter((f): f is ActiveFeature => f !== null) // фильтруем null и сохраняем тип |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment is written in Russian ('фильтруем null и сохраняем тип' means 'filter null and preserve type'). Project comments should be in English for consistency. Replace with: 'filter out null values and preserve type'.
| .filter((f): f is ActiveFeature => f !== null) // фильтруем null и сохраняем тип | |
| .filter((f): f is ActiveFeature => f !== null) // filter out null values and preserve type |
| <div className='relative w-64'> | ||
| <input | ||
| type="text" | ||
| value={search} | ||
| onChange={handleSearchChange} | ||
| placeholder="Search crags..." | ||
| className='input input-bordered w-full' | ||
| /> | ||
| {suggestions.length > 0 && ( | ||
| <ul className='absolute top-full left-0 w-full bg-white shadow-lg rounded-md max-h-48 overflow-y-auto z-50'> | ||
| {suggestions.map(c => ( | ||
| <li | ||
| key={c.data.id} | ||
| onClick={() => handleSelect(c)} | ||
| className='cursor-pointer p-2 hover:bg-gray-200' | ||
| > | ||
| {c.data.areaName} | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| )} | ||
| </div> |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent indentation. This div has extra leading spaces compared to surrounding elements. It should align with the Checkbox components at the same nesting level.
| <div className='relative w-64'> | |
| <input | |
| type="text" | |
| value={search} | |
| onChange={handleSearchChange} | |
| placeholder="Search crags..." | |
| className='input input-bordered w-full' | |
| /> | |
| {suggestions.length > 0 && ( | |
| <ul className='absolute top-full left-0 w-full bg-white shadow-lg rounded-md max-h-48 overflow-y-auto z-50'> | |
| {suggestions.map(c => ( | |
| <li | |
| key={c.data.id} | |
| onClick={() => handleSelect(c)} | |
| className='cursor-pointer p-2 hover:bg-gray-200' | |
| > | |
| {c.data.areaName} | |
| </li> | |
| ))} | |
| </ul> | |
| )} | |
| </div> | |
| <div className='relative w-64'> | |
| <input | |
| type="text" | |
| value={search} | |
| onChange={handleSearchChange} | |
| placeholder="Search crags..." | |
| className='input input-bordered w-full' | |
| /> | |
| {suggestions.length > 0 && ( | |
| <ul className='absolute top-full left-0 w-full bg-white shadow-lg rounded-md max-h-48 overflow-y-auto z-50'> | |
| {suggestions.map(c => ( | |
| <li | |
| key={c.data.id} | |
| onClick={() => handleSelect(c)} | |
| className='cursor-pointer p-2 hover:bg-gray-200' | |
| > | |
| {c.data.areaName} | |
| </li> | |
| ))} | |
| </ul> | |
| )} | |
| </div> |
| setClickInfo(prev => { | ||
| if (prev) setActiveFeatureVisual(prev, { selected: false, hover: false }) | ||
| setActiveFeatureVisual(crag, { selected: true, hover: false }) | ||
| return crag | ||
| }) |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setClickInfo callback is using the previous state incorrectly. The callback receives prev but doesn't use it in the return logic - it always returns crag regardless of the previous value. This should likely be a direct call: setClickInfo(crag) followed by the visual state updates, or the callback should properly handle the state transition.
| setClickInfo(prev => { | |
| if (prev) setActiveFeatureVisual(prev, { selected: false, hover: false }) | |
| setActiveFeatureVisual(crag, { selected: true, hover: false }) | |
| return crag | |
| }) | |
| if (clickInfo) setActiveFeatureVisual(clickInfo, { selected: false, hover: false }) | |
| setActiveFeatureVisual(crag, { selected: true, hover: false }) | |
| setClickInfo(crag) |
| const features = mapInstance.querySourceFeatures('crags', { sourceLayer: 'crags' }) | ||
| const cragsFeatures = features | ||
| .map(f => tileToFeature('crag-name-labels', {x:0,y:0}, f.geometry, f.properties as TileProps, mapInstance)) |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic values {x:0, y:0} are passed to tileToFeature without explanation. Consider extracting this as a named constant (e.g., DEFAULT_TILE_COORDS) or adding a comment explaining why these coordinates are used when querying all crags.
This Pull request Fixes #1184
Adding rock search and selection functions.
Type of PR.
Description
Added search and selection functionality for crags in the map toolbar:
What this PR achieves
Screenshots
Search and Suggestions
Marking a selected crag