Add side menu to Entity page - #546
Draft
jpherr wants to merge 3 commits into
Draft
Conversation
Contributor
WalkthroughEntity responses now expose parsed coordinates, while dataset and entity pages use a shared side-navigation macro. The entity page also adopts a two-column layout, GOV.UK table rendering, revised geometry placement, and conditional navigation links with unit and integration coverage. ChangesEntity navigation and geometry
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant EntityRoute
participant get_entity_lat_lng
participant entity.html
participant appSideNav
EntityRoute->>get_entity_lat_lng: Parse entity point
get_entity_lat_lng-->>EntityRoute: Return latitude and longitude
EntityRoute->>entity.html: Provide entity and coordinate context
entity.html->>appSideNav: Pass conditional sideNavSections
appSideNav-->>entity.html: Render navigation links
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
application/templates/components/side-nav/macro.jinja (1)
1-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive the
<aside>landmark an accessible name.The outer
role="complementary"region has noaria-label/aria-labelledby, unlike the GOV.UK Design System's "related items" reference pattern (which typically labels the wrapper, e.g. via a heading such as "Related content"). Since this macro is reused on both the dataset and entity pages, an unlabelled landmark makes landmark-based navigation less useful for screen-reader users.♿ Proposed fix
- <aside class="app-related-items" role="complementary"> + <aside class="app-related-items" role="complementary" aria-label="Related content">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/templates/components/side-nav/macro.jinja` around lines 1 - 30, Update the appSideNav macro’s outer complementary aside to provide an accessible name, using an appropriate existing heading or a concise related-content label while preserving the current section navigation structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@application/templates/components/side-nav/macro.jinja`:
- Around line 1-30: Update the appSideNav macro’s outer complementary aside to
provide an accessible name, using an appropriate existing heading or a concise
related-content label while preserving the current section navigation structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fd902bb5-1eea-461f-8eb8-baea06e804cf
📒 Files selected for processing (7)
application/routers/entity.pyapplication/templates/components/side-nav/macro.jinjaapplication/templates/dataset.htmlapplication/templates/entity.htmlassets/stylesheets/component/_table.scsstests/integration/test_entity.pytests/unit/routers/test_entity.py
jpherr
force-pushed
the
686-add-side-menu-to-entity-page
branch
from
July 30, 2026 07:11
9342502 to
fdbc596
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
application/templates/entity.html (1)
263-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate "specification"/"data-design" side-nav section logic across templates. Both templates independently build identical "Dataset definition" and "Designing the data" side-nav sections (same headings, hint text, and href patterns), risking drift if one is updated without the other.
application/templates/entity.html#L263-L280: extract the "specification"/"data-design" section-building into a shared Jinja macro (e.g.commonDatasetSideNavSections(dataset)incomponents/side-nav/macro.jinja) and call it here.application/templates/dataset.html#L228-L245: call the same shared macro instead of re-declaring the identical sections.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/templates/entity.html` around lines 263 - 280, Extract the duplicated “specification” and conditional “data-design” side-nav section construction into a shared Jinja macro such as commonDatasetSideNavSections in components/side-nav/macro.jinja, preserving the existing headings, hints, links, and dataset.consideration condition. Update application/templates/entity.html lines 263-280 and application/templates/dataset.html lines 228-245 to call the shared macro instead of defining these sections independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@application/templates/entity.html`:
- Around line 263-280: Guard all dataset-dependent side-navigation content in
the entity template with the existing dataset truthiness check. Ensure the “On a
map” link using dataset.dataset and the “Dataset definition” and “Designing the
data” sections are only rendered when dataset exists, while preserving the
consideration check within that guard.
---
Nitpick comments:
In `@application/templates/entity.html`:
- Around line 263-280: Extract the duplicated “specification” and conditional
“data-design” side-nav section construction into a shared Jinja macro such as
commonDatasetSideNavSections in components/side-nav/macro.jinja, preserving the
existing headings, hints, links, and dataset.consideration condition. Update
application/templates/entity.html lines 263-280 and
application/templates/dataset.html lines 228-245 to call the shared macro
instead of defining these sections independently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 47bea2b1-6c46-403b-a888-6a997b77ceb0
📒 Files selected for processing (6)
application/routers/entity.pyapplication/templates/components/side-nav/macro.jinjaapplication/templates/dataset.htmlapplication/templates/entity.htmltests/integration/test_entity.pytests/unit/routers/test_entity.py
jpherr
force-pushed
the
686-add-side-menu-to-entity-page
branch
from
July 30, 2026 07:54
fdbc596 to
5befa7f
Compare
Context: The data HTML tables on the Dataset and Entity pages differ both visually and in code despite serving largely the same purpose. The Dataset page makes use of the official GovUk Table component. This change replaces our own table markup and instead imports the GovUk table. Because we do have an additional column for facts links, I have preserved a hidden header row to allow screenreaders to read the heading for the facts column, hence the additional CSS. The first column is scoped to row, which declares this column as row headings, as per a recent Accessibility audit (previously on the Dataset page we just used bold text instead of proper headings, failing WCAG 2.2 requirements). Changes: - Refactor Entity page to use GovUk table component
Context: Reposition the geographical-area map card from the bottom of the page to the top of the main content column, ahead of the record table, so it's visible without scrolling. The map now sits in the left column, does not consume the full width of the page, and as such, the height has also being reduced, cleaning up the UI. The new side-navigation that is being introduced in a separate commit will contain a link to the dedicated map page containing a full size map. Because we are adding a side-nav to this page that will include data download links, we no longer require the link on the embedded map, so this is being removed. Changes: - Move map to top of page in left hand column - Remove GeoJson download link from map
Context: The Dataset page has a side navigation which allows the user to easily access links to download files of the dataset data. The Entity page does not currently have this. We want the user to have easy access to downloadable files and a consistent UI. This change adds a side menu to the Entity page. Unlike on the Dataset page The download links return a response from our API where the file downloads reside as physical files on a separate service, here we return a response from the API and force a file download via the HTML download attribute. As such, it will only offer Json and GeoJson formats. This behaviour is the same as what was already in place with a link on the map for GeoJson. The map link in the side-nav uses lat,lng coords to link to the map page. To facilitate the use of the side-nav on both pages I have created a shared side-nav macro template, now used by both the Dataset and Entity pages. Changes: - Create Side nav template - Refactor Dataset page to use template - Apply template to Entity page - Add tests
jpherr
force-pushed
the
686-add-side-menu-to-entity-page
branch
from
July 30, 2026 12:06
5befa7f to
6f76274
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
application/templates/dataset.html (1)
228-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "specification"/"data-design" side-nav sections into a reusable macro. Both pages build near-identical "Dataset definition" and "Designing the data" side-nav sections from
dataset; one root cause is the lack of a shared builder for these common sections.
application/templates/dataset.html#L228-L246: replace this inline section-building with a call to a new macro (e.g.appDatasetDefinitionSections(dataset)) that returns the "specification" and optional "data-design" section dicts.application/templates/entity.html#L263-L282: replace the equivalent inline blocks (currently guarded additionally by{% if dataset %}) with the same shared macro call, passingdataset.♻️ Proposed shared macro sketch
{% macro appDatasetDefinitionSections(dataset) %} {% set sections = [] %} {% if dataset %} {% do sections.append({ "id": "specification", "heading": "Dataset definition", "hint": "You can view the definition of this dataset including the list of fields", "links": [ {"text": "Dataset definition for " ~ dataset.name ~ " dataset", "href": "https://digital-land.github.io/specification/dataset/" ~ dataset.dataset} ] }) %} {% if dataset.consideration %} {% do sections.append({ "id": "data-design", "heading": "Designing the data", "hint": "You can see details about how this dataset has been designed for planning.data.gov.uk", "links": [ {"text": dataset.consideration ~ " planning consideration", "href": "https://design.planning.data.gov.uk/planning-consideration/" ~ dataset.consideration} ] }) %} {% endif %} {% endif %} {{ sections }} {% endmacro %}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/templates/dataset.html` around lines 228 - 246, Extract the shared dataset side-nav section builder into an appDatasetDefinitionSections macro, preserving the specification section and optional data-design section when dataset.consideration exists. Replace the inline construction in application/templates/dataset.html lines 228-246 and application/templates/entity.html lines 263-282 with calls to this macro, passing dataset; the macro must retain entity.html’s existing behavior of producing no sections when dataset is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@application/templates/dataset.html`:
- Around line 228-246: Extract the shared dataset side-nav section builder into
an appDatasetDefinitionSections macro, preserving the specification section and
optional data-design section when dataset.consideration exists. Replace the
inline construction in application/templates/dataset.html lines 228-246 and
application/templates/entity.html lines 263-282 with calls to this macro,
passing dataset; the macro must retain entity.html’s existing behavior of
producing no sections when dataset is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f144454-de7c-4e5d-a2ca-ebd3edb84651
📒 Files selected for processing (7)
application/routers/entity.pyapplication/templates/components/side-nav/macro.jinjaapplication/templates/dataset.htmlapplication/templates/entity.htmlassets/stylesheets/component/_table.scsstests/integration/test_entity.pytests/unit/routers/test_entity.py
🚧 Files skipped from review as they are similar to previous changes (1)
- assets/stylesheets/component/_table.scss
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this? (check all applicable)
Description
Please replace this line with a brief description of the changes made.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above. Please refer to the Digital Land Testing Guidance for more information.
have not been included
[optional] Are there any post deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit