Skip to content

City planning: geojson properties documentation, fixes on UX for history#205

Merged
peterdrier merged 11 commits intopeterdrier:mainfrom
davinov:fix-sound-zones-property
Apr 10, 2026
Merged

City planning: geojson properties documentation, fixes on UX for history#205
peterdrier merged 11 commits intopeterdrier:mainfrom
davinov:fix-sound-zones-property

Conversation

@davinov
Copy link
Copy Markdown

@davinov davinov commented Apr 10, 2026

  • Document the expected properties in the geojson files to upload
  • Multiple fixes for the history panel
    • history button is accessible when clicking on a barrio, in the popup (like the edit button)
    • everyone can access it and preview it, but only editors can restore versions
    • previewed zones are validated (check with the other barrios and the limit zones), but avoid conflict with their current version
  • Move the admin button to the top right to avoid colliding with the rest of the action bar at the bottom

davinov and others added 9 commits April 10, 2026 08:14
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
History (and Restore for editors) is now accessible from the zone click
popup, removing the need to enter edit mode first. The toolbar history
button is removed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Track previewCampSeasonId in state so overlapsOtherCamps excludes the
barrio whose history is being previewed, matching the existing behavior
for edit mode via activeCampSeasonId.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@peterdrier
Copy link
Copy Markdown
Owner

Code Review

Overall: Looks good — well-structured improvements to history panel UX, proper event listener attachment, and good localization coverage. A few items worth discussing below.

Potential Issue: Hardcoded English strings in JavaScript

  • edit.js: The history panel title is set to History of ${campName} (hardcoded English). Since the rest of the page is localized via @Localizer[...], this stands out. The panel's default title from the partial (_HistoryOffcanvas.cshtml) uses @Localizer["CityPlanning_HistoryTitle"], but the JS override doesn't go through localization. Consider passing the localized format string via a data-* attribute on the panel element or via the CONFIG object, e.g. data-history-title-template with a {0} placeholder.
  • Similarly, strings like "No history yet.", "Preview", "Restore", and "Restore this version?" in edit.js are hardcoded English. These are pre-existing, but since this PR touches the history feature, it may be a good time to extract them.

Authorization: canEdit vs IS_MAP_ADMIN for Restore

  • edit.js (line ~201/237): The Restore button visibility changed from CONFIG.IS_MAP_ADMIN to the canEdit parameter. This is a deliberate design change (camp leads who can edit their own polygon can also restore their own history) and the description confirms this intent. The server-side CanUserEditAsync check on the restore endpoint protects against unauthorized use, so this is safe. Good change.

Defensive coding improvement

  • edit.js clearDrawLabel(): Adding optional chaining (?.) to map.getSource(...) calls is a good defensive improvement. This prevents crashes if sources haven't been added yet (e.g., during initialization or if limit zone data is absent).

Minor: Nested wrapper divs in admin link

  • Index.cshtml (new admin panel link): The markup has three levels of nesting (position-absolute > card > card-body > d-flex > d-flex) for a single button. This could be simplified to fewer wrapper divs, though it works fine as-is.

Note: GeoJSON property rename (sound_zone -> SoundZone)

  • layers.js: This PR renames the GeoJSON property from sound_zone to SoundZone. If any limit zone GeoJSON files have already been uploaded to the database with sound_zone as the property name, those will stop rendering colored sound zone borders. This is a breaking change for existing data. Either existing data needs to be re-uploaded, or the code should support both property names during a transition period (e.g., f.properties?.SoundZone || f.properties?.sound_zone).

Suggestions

  • The loadHistory function doesn't check resp.ok before calling resp.json(). If the API returns an error status, this will throw. Consider adding a guard: if (!resp.ok) { list.innerHTML = '<p class="text-danger">Failed to load history.</p>'; return; }. (This is a pre-existing pattern, not introduced by this PR.)
  • The previewCampSeasonId state management is clean — setting it on preview click and clearing it on offcanvas close with { once: true } is well done.

Thanks for the contribution! 🎉

@peterdrier
Copy link
Copy Markdown
Owner

-- Peter here, don't worry about the localization yet, I haven't figured out a good way to do that in JS yet (unless you know one?) The rest are more informational.. I can wrap this up later when I get to merging..

peterdrier and others added 2 commits April 10, 2026 22:29
- Support both SoundZone (PascalCase) and sound_zone (snake_case) GeoJSON
  property names to avoid breaking existing uploaded data
- Add resp.ok guard in loadHistory to show error message on API failure
- Simplify admin panel link markup by removing unnecessary wrapper divs

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Keep history button from main in bottom toolbar, keep admin link
at top-right from PR branch (no duplicate bottom admin link).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@peterdrier peterdrier merged commit 1c93c69 into peterdrier:main Apr 10, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants