feat: add viz poc 1#963
Conversation
nicolaskempf57
force-pushed
the
feat_add_viz_poc_1
branch
from
0d4cfa5 to
e6e3f61
Compare
function as source of truth
- Convert resources to computed from savedResources - Add loadChart() to fetch chart and extract resource IDs from series - Add loadMissingResourcesForChart() to fetch individual resources - Add loadSelectedChart() triggered by Charger button - Switch form from reactive to ref - Move dataset initialization to top level - Fetch charts list at top level instead of onMounted
… debounced preview
Also add translations
| watch(() => props.chart.series, async () => { | ||
| await fetchSeriesProfile() | ||
| }, { immediate: true, deep: true }) | ||
|
|
||
| // Watch for changes in the chart or its series | ||
| watch([() => props.chart.series, () => props.chart.x_axis.column_x], async () => { | ||
| await fetchSeriesData() | ||
| }, { immediate: true, deep: true }) |
There was a problem hiding this comment.
fetchSeriesProfile and fetchSeriesData run in parallel I think and they both reset and write in status.value and error.value, one error followed by one success will override the status?
There was a problem hiding this comment.
I added a simple count to have a success only when all succeed
There was a problem hiding this comment.
I'm not sure it solves all the problems with this design. Setting status = 'success' after fetching the profile will show a flash of the chart without dataset then fetchSeriesData resolve and the chart is populated? Maybe remove status = 'success' and let the watch(pendingOperation) set it to be sure the two fetches are done before setting the status or maybe refactor this logic to be simpler? fetchSeriesProfile and fetchSeriesData are only called in these watchers? You may merge them together and call with a Promise.all(). If you don't want to get the profile on props.chart.x_axis.column_x you may just have one function with an argument dataOnly=true, it will solves all these problems, no?
| <button | ||
| class="fr-btn" | ||
| :disabled="!selectedChartId" | ||
| @click="loadSelectedChart" | ||
| > | ||
| {{ $t('Charger') }} | ||
| </button> |
There was a problem hiding this comment.
Missing type="button", no?
| const formatter = new Intl.NumberFormat('fr-FR') | ||
| for (const param of params) { | ||
| const seriesName = param.seriesName | ||
| const seriesConfig = props.chart.series.find(s => s.column_y === seriesName) |
There was a problem hiding this comment.
If there is two series with the same column name (two different aggregate sum/mean for exemple), you'll always get the first one and then ${seriesName}__${seriesConfig.aggregate_y} could match nothing? (the seriesName from the one you hover but the seriesConfig.aggregate_y from the first serie?
| watch(() => props.chart.series, async () => { | ||
| await fetchSeriesProfile() | ||
| }, { immediate: true, deep: true }) | ||
|
|
||
| // Watch for changes in the chart or its series | ||
| watch([() => props.chart.series, () => props.chart.x_axis.column_x], async () => { | ||
| await fetchSeriesData() | ||
| }, { immediate: true, deep: true }) |
There was a problem hiding this comment.
I'm not sure it solves all the problems with this design. Setting status = 'success' after fetching the profile will show a flash of the chart without dataset then fetchSeriesData resolve and the chart is populated? Maybe remove status = 'success' and let the watch(pendingOperation) set it to be sure the two fetches are done before setting the status or maybe refactor this logic to be simpler? fetchSeriesProfile and fetchSeriesData are only called in these watchers? You may merge them together and call with a Promise.all(). If you don't want to get the profile on props.chart.x_axis.column_x you may just have one function with an argument dataOnly=true, it will solves all these problems, no?
| } | ||
|
|
||
| function removeFilter(index: number) { | ||
| if (!form.value.filter || !('filters' in form.value.filter)) return |
There was a problem hiding this comment.
removeFilter returns early when form.value.filter is a bare Filter
(not an AndFilters), but filterList (line 597) still renders a row
with its delete button in that case — clicking it is a silent no-op.
The form lands in this state when loading an existing chart whose
series[0].filters is a bare Filter: toChartForm (charts.ts:4)
casts it as Filter | null and assigns it directly to
form.value.filter, even though DataSeries.filters is typed as
GenericFilter and can be either shape at runtime.
Two options:
-
normalize in
toChartFormsoform.value.filteris always either
nullor anAndFilters(thenfilterListandremoveFilterlose
the dual-shape branching), -
or handle the bare-Filter case here:
if (!form.value.filter) return
if (!('filters' in form.value.filter)) {
if (index === 0) form.value.filter = null
return
}
form.value.filter.filters.splice(index, 1)
...
The normalization in toChartForm is cleaner.
| if (valA === null && valB === null) return 0 | ||
| if (valA === undefined && valB === undefined) return 0 |
There was a problem hiding this comment.
The "both null" / "both undefined" checks on lines 72-73 are unreachable:
lines 70-71 already return as soon as either side is null/undefined, so
when both values are null the comparator returns -1 (asc) instead of 0 —
a violation of comparator equality. Modern engines use a stable sort so
the practical impact is limited, but the dead checks reveal the intent
doesn't match the behavior.
Either drop lines 72-73, or (cleaner) handle equality first:
const aNullish = valA === null || valA === undefined
const bNullish = valB === null || valB === undefined
if (aNullish && bNullish) return 0
if (aNullish) return sortDirection === 'asc' ? -1 : 1
if (bNullish) return sortDirection === 'asc' ? 1 : -1
3 participants
Fixes datagouv/data.gouv.fr#1972