feat(charts): add In view filter to chart list#352
Conversation
|
Thanks for the contribution! Are you still working on this (it's marked as a draft). Let me know... |
joelkoz
added
awaiting response
feat(charts): add "In view" filter to chart list Adds a slide toggle to the chart list that filters the displayed charts to only those whose bounds intersect the current map viewport. The list updates reactively as the user pans/zooms the map. The text filter and the in-view filter can be combined. Charts without bounds metadata are treated as global and remain visible. The viewport-intersection test lives in a pure isChartInView() helper in chart-utils, alongside extentFromBounds, with unit tests covering overlap, containment, edge-touching, disjoint and missing-bounds cases. @
msallin
force-pushed
the
claude/chart-bounds-filter
branch
from
40ce9c9 to
d0ed9c9
Compare
📝 WalkthroughWalkthroughThis PR adds chart-list filtering to show only charts whose bounds intersect the current map viewport. It introduces ChangesChart list viewport filtering
Sequence Diagram(s)sequenceDiagram
participant User
participant ChartListComponent
participant MapExtent
participant isChartInView
User->>ChartListComponent: toggleInViewOnly(true)
ChartListComponent->>ChartListComponent: doFilter()
ChartListComponent->>MapExtent: read app.mapExtent()
MapExtent-->>ChartListComponent: current extent
ChartListComponent->>isChartInView: intersects(bounds, extent)
isChartInView-->>ChartListComponent: visible / hidden
MapExtent->>ChartListComponent: extent changes on pan or zoom
ChartListComponent->>ChartListComponent: effect reruns doFilter()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/app/modules/map/ol/lib/charts/chart-utils.spec.ts(node:2) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignore-files Oops! Something went wrong! :( ESLint: 10.5.0 /.eslintrc:2 SyntaxError: Unexpected token ':' src/app/modules/map/ol/lib/charts/chart-utils.ts(node:2) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignore-files Oops! Something went wrong! :( ESLint: 10.5.0 /.eslintrc:2 SyntaxError: Unexpected token ':' src/app/modules/skresources/components/charts/chartlist.ts(node:2) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignore-files Oops! Something went wrong! :( ESLint: 10.5.0 /.eslintrc:2 SyntaxError: Unexpected token ':' 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 |
msallin
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/app/modules/map/ol/lib/charts/chart-utils.ts`:
- Around line 41-57: isChartInView currently calls intersects directly, so
wrapped viewport extents crossing the antimeridian can be misclassified; update
this helper to detect a wrapped extent and split or normalize it into two
longitude ranges before testing overlap. Keep the existing global-chart fallback
for undefined bounds, and add a regression test around isChartInView covering a
dateline-crossing view to verify charts near +/-180 are classified correctly.
In `@src/app/modules/skresources/components/charts/chartlist.html`:
- Around line 107-117: The icon-only action buttons in chartlist.html no longer
have an explicit accessible name, so add an aria-label or visible text to both
button elements near showChartLayers and the adjacent action button. Keep the
existing matTooltip if desired, but ensure each control has a unique,
descriptive label for assistive tech that matches its action.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d12843b-161a-4c34-81e6-8f7644b50a2e
📒 Files selected for processing (4)
src/app/modules/map/ol/lib/charts/chart-utils.spec.tssrc/app/modules/map/ol/lib/charts/chart-utils.tssrc/app/modules/skresources/components/charts/chartlist.htmlsrc/app/modules/skresources/components/charts/chartlist.ts
| * Limitation: this is a plain min/max overlap test and does not account for a | ||
| * viewport that crosses the antimeridian (+/-180 longitude); near the dateline a | ||
| * chart may be wrongly included or excluded. | ||
| * | ||
| * Example: | ||
| * bounds=[10, 40, 20, 50], extent=[15, 45, 30, 60] -> true (overlap) | ||
| * bounds=[10, 40, 20, 50], extent=[30, 45, 40, 60] -> false (disjoint) | ||
| * bounds=undefined, extent=[...] -> true (global chart) | ||
| */ | ||
| export function isChartInView( | ||
| bounds: number[] | undefined, | ||
| extent: Extent | ||
| ): boolean { | ||
| if (!Array.isArray(bounds) || bounds.length !== 4) { | ||
| return true; | ||
| } | ||
| return intersects(bounds, extent); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Handle wrapped view extents before intersecting.
This helper is documented to return wrong results when the viewport crosses the antimeridian, so the new In view filter will misclassify charts for dateline-adjacent views. Split wrapped extents into two longitude ranges (or normalize them first) before calling intersects, and add a regression test for that case.
🤖 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 `@src/app/modules/map/ol/lib/charts/chart-utils.ts` around lines 41 - 57,
isChartInView currently calls intersects directly, so wrapped viewport extents
crossing the antimeridian can be misclassified; update this helper to detect a
wrapped extent and split or normalize it into two longitude ranges before
testing overlap. Keep the existing global-chart fallback for undefined bounds,
and add a regression test around isChartInView covering a dateline-crossing view
to verify charts near +/-180 are classified correctly.
| <button | ||
| mat-button | ||
| mat-icon-button | ||
| (click)="showChartLayers(true)" | ||
| matTooltip="Re-order Charts" | ||
| matTooltipPosition="above" | ||
| > | ||
| <mat-icon>import_export</mat-icon> | ||
| Re-order | ||
| </button> | ||
|
|
||
| <button | ||
| mat-button | ||
| mat-icon-button |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore an accessible name for the icon-only actions.
Changing these controls to icon-only buttons removes their explicit label for assistive tech. Add an aria-label (or visible text) for both actions; matTooltip is not enough here.
Suggested fix
<button
mat-icon-button
+ aria-label="Re-order Charts"
(click)="showChartLayers(true)"
matTooltip="Re-order Charts"
matTooltipPosition="above"
>
<mat-icon>import_export</mat-icon>
</button>
<button
mat-icon-button
+ aria-label="Add Chart"
matTooltip="Add Chart"
matTooltipPosition="above"
[matMenuTriggerFor]="addchartmenu"
>
<mat-icon>add</mat-icon>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| mat-button | |
| mat-icon-button | |
| (click)="showChartLayers(true)" | |
| matTooltip="Re-order Charts" | |
| matTooltipPosition="above" | |
| > | |
| <mat-icon>import_export</mat-icon> | |
| Re-order | |
| </button> | |
| <button | |
| mat-button | |
| mat-icon-button | |
| <button | |
| mat-icon-button | |
| aria-label="Re-order Charts" | |
| (click)="showChartLayers(true)" | |
| matTooltip="Re-order Charts" | |
| matTooltipPosition="above" | |
| > | |
| <mat-icon>import_export</mat-icon> | |
| </button> | |
| <button | |
| mat-icon-button | |
| aria-label="Add Chart" | |
| matTooltip="Add Chart" | |
| matTooltipPosition="above" | |
| [matMenuTriggerFor]="addchartmenu" | |
| > | |
| <mat-icon>add</mat-icon> | |
| </button> |
🤖 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 `@src/app/modules/skresources/components/charts/chartlist.html` around lines
107 - 117, The icon-only action buttons in chartlist.html no longer have an
explicit accessible name, so add an aria-label or visible text to both button
elements near showChartLayers and the adjacent action button. Keep the existing
matTooltip if desired, but ensure each control has a unique, descriptive label
for assistive tech that matches its action.
|
Thanks @msallin — this is a clean, well-tested addition, and I really like that you documented the antimeridian limitation right in the JSDoc rather than papering over it. The Two things before I merge:
Once the button labels are in I'm good to merge. Thanks again for the contribution! |
2 participants
Summary
boundsintersect the current map viewport (app.mapExtent) are shown.effectthat recomputes the filter while the toggle is on.Changes
chart-utils.ts: new pureisChartInView(bounds, extent)helper (alongsideextentFromBounds) encapsulating the bounds/viewport intersection and the "no bounds = global" rule. Both inputs are EPSG:4326[minLon, minLat, maxLon, maxLat], compared viaintersectsfromol/extentwith no re-projection.chart-utils.spec.ts: unit tests forisChartInView(overlap, containment, edge-touching, disjoint, missing/malformed bounds) plus boundary tests forextentFromBounds.chartlist.ts: newinViewOnlyflag,toggleInViewOnly(), an overriddendoFilter()combining the name filter with the viewport filter, and aneffect()watchingapp.mapExtent.chartlist.html: new In viewmat-slide-toggle; the existing boundary toggle relabelled Bounds and the Re-order / Add buttons made icon-only to fit on one row.Known limitation
The intersection is a plain min/max overlap test and does not handle viewports crossing the antimeridian (±180° longitude); this is documented in
isChartInView.Closes
Closes #351
Test plan
Summary by CodeRabbit
New Features
Bug Fixes