From a73e5cda6a6e264a2f39f83ed7956018f9dfbb7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A2u=20Cao?= Date: Tue, 27 Jan 2026 13:11:34 +0700 Subject: [PATCH] Clean up code comments --- app/components/map.gjs | 71 +++++-------------------------- app/components/places-sidebar.gjs | 6 --- app/services/storage.js | 18 +------- 3 files changed, 12 insertions(+), 83 deletions(-) diff --git a/app/components/map.gjs b/app/components/map.gjs index 3f6799e..6d82406 100644 --- a/app/components/map.gjs +++ b/app/components/map.gjs @@ -28,8 +28,8 @@ export default class MapComponent extends Component { searchOverlayElement; selectedPinOverlay; selectedPinElement; - crosshairElement; // New crosshair - crosshairOverlay; // New crosshair overlay + crosshairElement; + crosshairOverlay; setupMap = modifier((element) => { if (this.mapInstance) return; @@ -114,18 +114,13 @@ export default class MapComponent extends Component { this.mapInstance.addOverlay(this.searchOverlay); // Selected Pin Overlay (Red Marker) - // We create the element in the template (or JS) and attach it. - // Using JS creation to ensure it's cleanly managed by OpenLayers this.selectedPinElement = document.createElement('div'); this.selectedPinElement.className = 'selected-pin-container'; // Create the icon structure inside const pinIcon = document.createElement('div'); pinIcon.className = 'selected-pin'; - // We can't use the Glimmer component easily inside a raw DOM element created here. - // So we'll inject the SVG string directly or mount it. - // Feather icons are globally available if we used the script, but we are using the module approach. - // Simple SVG for Map Pin: + // Simple SVG for Map Pin pinIcon.innerHTML = ``; const pinShadow = document.createElement('div'); @@ -136,7 +131,7 @@ export default class MapComponent extends Component { this.selectedPinOverlay = new Overlay({ element: this.selectedPinElement, - positioning: 'bottom-center', // Important: Pin tip is at the bottom + positioning: 'bottom-center', // Pin tip is at the bottom stopEvent: false, // Let clicks pass through }); this.mapInstance.addOverlay(this.selectedPinOverlay); @@ -144,26 +139,17 @@ export default class MapComponent extends Component { // Crosshair Overlay (for Creating New Place) this.crosshairElement = document.createElement('div'); this.crosshairElement.className = 'map-crosshair'; - // Use an SVG or simple CSS cross this.crosshairElement.innerHTML = ` `; - // We attach it to the map control container OR keep it as an overlay centered on map center? - // Actually, a fixed center overlay is trickier in OpenLayers because Overlays move with the map. - // If we want it FIXED in the center of the VIEWPORT, it should be a Control or just an absolute HTML element on top of the map div. - // Adding it as a Control is cleaner. - - // HOWEVER, the request says "cross hair drawn on the map... which should be removed when saving". - // A fixed element in the center of the screen is best for "choose location by dragging map". - // So let's append it to the map container directly via Glimmer template or JS. - - // We'll append it to the map target element (this.element is the target). element.appendChild(this.crosshairElement); + + // Geolocation Pulse Overlay this.locationOverlayElement = document.createElement('div'); this.locationOverlayElement.className = 'search-pulse blue'; @@ -306,7 +292,7 @@ export default class MapComponent extends Component { }; if (targetResolution) { - const maxResolution = view.getResolutionForZoom(17); // Use 17 as safe max zoom for accuracy < 20m + const maxResolution = view.getResolutionForZoom(17); viewOptions.resolution = Math.max(targetResolution, maxResolution); } else { viewOptions.zoom = 16; @@ -362,16 +348,9 @@ export default class MapComponent extends Component { this.mapInstance.getTarget().style.cursor = hit ? 'pointer' : ''; }); - // Load initial bookmarks this.storage.rs.on('ready', () => { - // Initial load based on current view this.handleMapMove(); }); - - // Listen for remote storage changes - // this.storage.rs.on('connected', () => { - // this.loadBookmarks(); - // }); }); // Track the selected place from the UI Service (Router -> Map) @@ -440,12 +419,9 @@ export default class MapComponent extends Component { const offsetPixels = height * 0.25; // Distance from desired pin pos to map center const offsetMapUnits = offsetPixels * resolution; - // Shift center SOUTH (decrease Y) + // Shift center SOUTH (decrease Y). // Note: In Web Mercator (EPSG:3857), Y increases North. - // So to look "lower", we decrease Y? No wait. - // If we move the camera South (decrease Y), the features move North (Up) on screen. - // We want the Pin (fixed lat/lon) to be Higher up on screen. - // So we must move the Camera South (Lower Y). + // To move the camera South (Lower Y), we subtract. targetCenter = [coords[0], coords[1] - offsetMapUnits]; } @@ -493,7 +469,6 @@ export default class MapComponent extends Component { } } - // Re-fetch bookmarks when the version changes (triggered by parent action or service) updateBookmarks = modifier(() => { // Depend on the tracked storage.placesInView to automatically update when they change const places = this.storage.placesInView; @@ -505,15 +480,10 @@ export default class MapComponent extends Component { if (!this.bookmarkSource) return; if (!places || places.length === 0) { - // Fallback or explicit check if we have tracked property usage? - // The service updates 'placesInView'. We should probably use that if we want reactiveness. places = this.storage.placesInView; } - // Previously: const places = await this.storage.places.getPlaces(); - // We no longer want to fetch everything blindly. // We rely on 'placesInView' being updated by handleMapMove calling storage.loadPlacesInBounds. - this.bookmarkSource.clear(); if (places && Array.isArray(places)) { @@ -557,15 +527,7 @@ export default class MapComponent extends Component { // we need to pan the map so those coordinates are UNDER the crosshair. const coords = this.mapUi.creationCoordinates; if (coords && coords.lat && coords.lon) { - // We only animate if the map center isn't already "roughly" correct - // But actually, updateCreationCoordinates is called by handleMapMove too. - // We need to distinguish "initial set" vs "drag update". - // The Service doesn't distinguish, but if we are just entering mode, - // we can check if the current map center aligns. - - // Better approach: - // We calculate where the map center *should* be to put the target coords - // under the crosshair. + // We only animate if the map center isn't already "roughly" correct. const targetCoords = fromLonLat([coords.lon, coords.lat]); this.animateToCrosshair(targetCoords); } @@ -599,22 +561,11 @@ export default class MapComponent extends Component { // We want 'targetCoords' to be at [crosshairPixelX, crosshairPixelY]. // If we center the map on 'targetCoords', it will be at [mapCenterX, mapCenterY]. // So we need to shift the map center by the OPPOSITE of the offset. - // Wait. - // If crosshair is to the right (+X), we need to move the camera LEFT (-X) to bring the point there? - // Let's think in map units. const view = this.mapInstance.getView(); const resolution = view.getResolution(); const offsetMapUnitsX = offsetX * resolution; - const offsetMapUnitsY = -offsetY * resolution; // Y is inverted in pixel vs map coords usually? - // In Web Mercator: Y increases North (Up). - // In Pixels: Y increases South (Down). - // So +PixelY (Down) = -MapY (South). Correct. - - // If crosshair is at +100px (Right), we want the target to be there. - // If we center on target, it is at 0px. - // To make it appear at +100px, we must shift the camera center by -100px (Left). - // So CenterX_new = TargetX - offsetMapUnitsX. + const offsetMapUnitsY = -offsetY * resolution; // Y is inverted in pixel vs map coords const targetX = targetCoords[0]; const targetY = targetCoords[1]; diff --git a/app/components/places-sidebar.gjs b/app/components/places-sidebar.gjs index c305ffa..61271f3 100644 --- a/app/components/places-sidebar.gjs +++ b/app/components/places-sidebar.gjs @@ -57,12 +57,7 @@ export default class PlacesSidebar extends Component { this.args.onBookmarkChange(); } - // Update selection to the new saved place object - // This updates the local UI state immediately without a route refresh if (this.args.onUpdate) { - // When deleting, we revert to a "fresh" object or just close. - // Since we close the sidebar below, we might not strictly need to update local state, - // but it's good practice. // Reconstruct the "original" place without ID/Geohash/CreatedAt const freshPlace = { ...place, @@ -75,7 +70,6 @@ export default class PlacesSidebar extends Component { // Also fire onSelect if it exists (for list view) if (this.args.onSelect) { - // Similar logic for select if needed, but we usually close. this.args.onSelect(null); } diff --git a/app/services/storage.js b/app/services/storage.js index 1a26f85..57326b5 100644 --- a/app/services/storage.js +++ b/app/services/storage.js @@ -74,18 +74,7 @@ export default class StorageService extends Service { handlePlaceChange(event) { const { newValue, relativePath } = event; - // Remove old entry if exists - // The relativePath is like "geohash/geohash/ULID" or just "ULID" depending on structure. - // Our structure is <2-char>/<2-char>/. - // But let's rely on the ID inside the object if possible, or extract from path. - - // We can't easily identify the ID from just relativePath without parsing logic if it's nested. - // However, for deletions (newValue is undefined), we might need the ID. - // Fortunately, our objects (newValue) contain the ID. - - // If it's a deletion, we need to find the object in our array to remove it. - // Since we don't have the ID in newValue (it's null), we rely on `relativePath`. - // Let's assume the filename is the ID. + // Extract ID from path (structure: <2-char>/<2-char>/) const pathParts = relativePath.split('/'); const id = pathParts[pathParts.length - 1]; @@ -141,7 +130,6 @@ export default class StorageService extends Service { ); if (missingPrefixes.length === 0) { - // console.debug('All prefixes already loaded for this view'); return; } @@ -151,10 +139,6 @@ export default class StorageService extends Service { await this.loadAllPlaces(missingPrefixes); // 4. Update our tracked list of loaded prefixes - // Using assignment to trigger reactivity if needed, though simple push/mutation might suffice - // depending on usage. Tracked arrays need reassignment or specific Ember array methods - // if we want to observe the array itself, but here we just check inclusion. - // Let's do a reassignment to be safe and clean. this.loadedPrefixes = [...this.loadedPrefixes, ...missingPrefixes]; this.currentBbox = bbox; }