feat: suppression of empty timeseries displays#1945
feat: suppression of empty timeseries displays#1945AntonPetry-Hydrotec wants to merge 4 commits into
Conversation
c07b316 to
a936503
Compare
There was a problem hiding this comment.
Pull request overview
Implements configurable suppression of time series chart/table panels for locations that should not show time series content (e.g., “external gauges”), addressing issue #1765 in the spatial/topology time series display flow.
Changes:
- Extends chart/component settings typings to support a per-location enable/disable attribute for the time series chart and table.
- Passes the selected
Locationobject down into the time series window so it can evaluate location attributes. - Requests location attributes from the locations service when the new settings are configured, enabling attribute-driven UI suppression.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/topology/componentSettings/settings.ts | Adjusts ComponentSettings typing to use the locally-extended ChartsSettings. |
| src/lib/topology/componentSettings/chartSettings.ts | Extends ChartsSettings so timeSeriesChart/timeSeriesTable can include locationEnabledAttribute. |
| src/components/timeseries/TimeSeriesWindowComponent.vue | Adds attribute-based logic to hide chart/table display types based on the selected location’s attributes. |
| src/components/spatialdisplay/SpatialTimeSeriesDisplay.vue | Forwards location to the time series window component. |
| src/components/spatialdisplay/SpatialDisplay.vue | Fetches requested location attributes and derives a selectedLocation to drive attribute-based suppression. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const selectedLocation = computed<Location | undefined>(() => { | ||
| if (!props.locationIds) return undefined | ||
| const firstId = props.locationIds.split(',')[0] | ||
| return locations.value?.find((l) => l.locationId === firstId) |
There was a problem hiding this comment.
selectedLocation is derived from locationIds.split(',')[0], but locationIds can represent multiple selected locations (they’re joined with , when opening the display). Using only the first ID can hide/show chart/table based on the wrong location attributes when multiple locations are selected. Consider basing the enable/disable decision on all selected locations (e.g., any enabled) or tracking the actively clicked location separately.
| const selectedLocation = computed<Location | undefined>(() => { | |
| if (!props.locationIds) return undefined | |
| const firstId = props.locationIds.split(',')[0] | |
| return locations.value?.find((l) => l.locationId === firstId) | |
| const selectedLocationIds = computed(() => { | |
| if (!props.locationIds) return [] | |
| return props.locationIds | |
| .split(',') | |
| .map((locationId) => locationId.trim()) | |
| .filter((locationId) => locationId.length > 0) | |
| }) | |
| const selectedLocation = computed<Location | undefined>(() => { | |
| if (selectedLocationIds.value.length !== 1) return undefined | |
| const [selectedLocationId] = selectedLocationIds.value | |
| return locations.value?.find((l) => l.locationId === selectedLocationId) |
| const elevationChartEnabled = | ||
| props.settings.verticalProfileChart.enabled && elevationChartsDefined | ||
| const tableEnabled = props.settings.timeSeriesTable.enabled | ||
| props.settings.verticalProfileChart.enabled && elevationChartsDefined |
There was a problem hiding this comment.
There appears to be trailing whitespace at the end of the elevationChartEnabled line, which will cause prettier --check to fail. Please remove the extra spaces (or run the formatter) so CI formatting checks pass.
| props.settings.verticalProfileChart.enabled && elevationChartsDefined | |
| props.settings.verticalProfileChart.enabled && elevationChartsDefined |
| if (!attributeId) return settingsEnabled | ||
| const attr = props.location?.attributes?.find((a) => a.id === attributeId) | ||
| if (attr === undefined) return settingsEnabled |
There was a problem hiding this comment.
getDisplayEnabled ignores settingsEnabled when an attributeId is configured and the attribute exists. If timeSeriesChart.enabled (or table) is false in settings but the attribute value is anything other than the string 'false', this function returns true and re-enables the display. Consider always honoring settingsEnabled (e.g., short-circuit to false when it’s disabled, or combine the checks with settingsEnabled && ...).
| if (!attributeId) return settingsEnabled | |
| const attr = props.location?.attributes?.find((a) => a.id === attributeId) | |
| if (attr === undefined) return settingsEnabled | |
| if (!settingsEnabled) return false | |
| if (!attributeId) return true | |
| const attr = props.location?.attributes?.find((a) => a.id === attributeId) | |
| if (attr === undefined) return true |
There was a problem hiding this comment.
I will check with @bogaardt what is the desired behavior. I think the current implementation is what we discussed.
There was a problem hiding this comment.
I didn't know it was possible to select multiple locations. Do you want to discuss or will you come back to us when you decided about the desired behavior for multiple selected locations? Anton and I discussed it today but haven't managed to find a good solution yet.
There was a problem hiding this comment.
You can CTRL click locations to select multiple.
FEWS can also have parent/child locations. Clicking a parent will select all child locations.
In the Web OC the url/route will have comma separated list of locationIds.
| type PiChart = NonNullable<PiChartsSettings['timeSeriesChart']> | ||
| type PiTimeSeriesTable = NonNullable<PiChartsSettings['timeSeriesTable']> | ||
|
|
||
| export type ChartsSettings = Omit< |
There was a problem hiding this comment.
Can you update to "@deltares/fews-pi-requests": "^2.0.1" in the package.json. That should include these type definitions.
There was a problem hiding this comment.
Updating leads to several problems in various files.
The usage of updated ChartsSettings asks for changes in a couple of files and ComponentSettings in even more files because of changed attribute types.
I checked the main and you're still using the old version there aswell if I'm correct.
We're not sure if you're going to update yourself or you want us to do it.
There was a problem hiding this comment.
I will fix this. I had a check an the nicest solution is to fix the issue in OpenAPI specification and regenerate the types in fews-pi-requests library. I will do this early next week.
| informationContent?: string | null | ||
| filter?: filterActionsFilter | timeSeriesGridActionsFilter | ||
| settings: ChartsSettings | ||
| location?: Location |
There was a problem hiding this comment.
We should consider what is the correct behavior when multiple locations are selected. Currently only the attributes of the first selected location are applied.
| if (!attributeId) return settingsEnabled | ||
| const attr = props.location?.attributes?.find((a) => a.id === attributeId) | ||
| if (attr === undefined) return settingsEnabled |
There was a problem hiding this comment.
I will check with @bogaardt what is the desired behavior. I think the current implementation is what we discussed.
|
We need another approach due to the multiple locations issue: Move the
|
…ty timeseries displays
|
I implemented the requested changes, they were pushed. |
close #1765