Skip to content

Reorder dive site details and move dives to sidebar - #263

Merged
kargig merged 1 commit into
mainfrom
feature/dive-site-details-layout-reorder
Sep 26, 2026
Merged

kargig merged 1 commit into
mainfrom
feature/dive-site-details-layout-reorder

Conversation

@kargig

@kargig kargig commented Sep 26, 2026

Copy link
Copy Markdown
Owner

Restructure the dive site details page left pane to follow:
Description -> Location -> Routes -> Marine Life -> Safety Information
-> Comments, giving priority to marine life before safety details.

Relocate the dives section to the right sidebar as "Recent Dives"
positioned between Diving Centers and Nearby Dive Sites:

  • Display up to 5 dives ordered chronologically by dive date
  • Add a "More..." button linking to /dives filtered by dive site
  • Add sort_by support to GET /api/v1/dive-sites/{id}/dives
  • Consolidate Nearby Dive Sites to display consistently on mobile

Enhance Diving Centers in the sidebar:

  • Make diving center titles clickable internal links to their detail
    pages within Divemap
  • Remove external contact links to encourage viewing full center info

Enhance static HTML and Markdown content generation:

  • Add ratings, aliases, tags, associated diving centers, and linked
    dive routes to server-side static HTML and LLM markdown exports
  • Eagerly load relationships in SEO router and content generator to
    prevent N+1 queries
  • Add test coverage for static HTML rendering

Restructure the dive site details page left pane to follow:
Description -> Location -> Routes -> Marine Life -> Safety Information
-> Comments, giving priority to marine life before safety details.

Relocate the dives section to the right sidebar as "Recent Dives"
positioned between Diving Centers and Nearby Dive Sites:
- Display up to 5 dives ordered chronologically by dive date
- Add a "More..." button linking to /dives filtered by dive site
- Add sort_by support to GET /api/v1/dive-sites/{id}/dives
- Consolidate Nearby Dive Sites to display consistently on mobile

Enhance Diving Centers in the sidebar:
- Make diving center titles clickable internal links to their detail
  pages within Divemap
- Remove external contact links to encourage viewing full center info

Enhance static HTML and Markdown content generation:
- Add ratings, aliases, tags, associated diving centers, and linked
  dive routes to server-side static HTML and LLM markdown exports
- Eagerly load relationships in SEO router and content generator to
  prevent N+1 queries
- Add test coverage for static HTML rendering
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Fragile Import

render_dive_site_main imports get_diving_center_slug and slugify from generate_static_content at call time. generate_static_content is a top-level script that imports boto3 and performs path manipulation at import time. If boto3 is not installed or the module is not importable in the runtime serving static HTML, this import raises and breaks rendering of every dive site page. Consider moving these helpers into a shared utility module instead of importing from the generation script.

from generate_static_content import get_diving_center_slug, slugify
Unvalidated Sort Param

sort_by is a free-form string; any value other than "rating" silently falls through to the recent-date ordering. This is not a security issue (the value is not interpolated into SQL), but it means typos like sort_by=ratings are silently accepted rather than rejected. Consider constraining it with an enum/Literal so invalid values return a 422.

sort_by: str = Query("recent", description="Sort order: 'recent' (dive date desc) or 'rating' (rating desc, then date desc)"),
Redundant Slice

The parent already requests limit=5 for recent dives, yet the sidebar also calls recentDives.slice(0, 5). This is harmless but redundant; more importantly, if the limit is ever raised, the sidebar silently truncates. Rely on a single source of truth for the count.

{recentDives.slice(0, 5).map(dive => (

@kargig
kargig merged commit 00c473a into main Sep 26, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant