refactor(components): ♻️ created a Banner component for displaying warnings or errors

[lloydrichards]

A stacked area chart isn't able to properly display negative values; that's why if it encounters some, results may look quite weird and confusing. We should display some kind of a warning / error that at least would let user know that it could be better to switch to a stacked bar chart or present some call to action to disable affected series from a chart. (#437)

To keep things generic and possibly reusable in the future, I've created a Banner component which will use the useChartState() to conditionally render a warning message at the top of the chart.

I'm open to suggestions on how to improve this and whether there might be a better place to throw or create this warning handeling.

Additionally, I needed to update the translations with a message for this warning so if there is a better way of doing this, le tme know 🙏

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
visualization-tool	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 7, 2023 7:54am

[ptbrowne]

LGTM, but I think a good secondary step should be to extract the business logic of extracting warnings + severity out of the chart staet, from the Banner itself. This way we could more easily test Banner against variants.

Something like this:

Here I find that from the parent, it is difficult to infer that the Banner goes over all the data to find warnings.

const WarningNegativeValues = () => <>
   <Trans id="dataset.error.negative-values">
      Careful, this dataset contains negative values and might not
      display correctly with this chart type.
      <br />
      <strong>Try using another chart type.</strong>
    </Trans>
</>

const getWarningFromChartState = chartState => {
	return hasNegativeValues(chartState) && area ? {
       severity : 'warning',
       Message: <WarningNegativeValues /> } : null 
}

const warning = useMemo(() => {
  return getWarningFromChartState(chartState)
})

return  <>
  { warning ? <Banner  warning={warning} /> : null }
  <Area />
   <Etc />
</>

I find it easier to see that Banner is responsible for warnings. What do you think ?

[lloydrichards]

What do you think ?

This was how I wanted to build it initially, but looking at the chart-area.tsx it seems like all the chart data is drilled down using providers, so wanted to keep the same pattern going. if the same data can be extracted from props, and the logic kept at the highest level, then maybe a hook that returns some kind of generic Failure object which can be passed to the Banner as props 🤔

[ptbrowne]

Yep ok, understood the need for coherence across charts, thanks for the explanation 👍 I have just seen the disabled eslint warning, and I think it could be a pitfall though, in general I think we should reserve disabling the exhaustive hook warning only for very specific cases, and I am not sure it is that useful here.

[ptbrowne]

(Just to be clear, I had put LGTM, so feel free to merge anytime)

[lloydrichards]

(Just to be clear, I had put LGTM, so feel free to merge anytime)

Thanks! I want to just wait til Bartosz is back and can have a look since it also relates to error/failure handling and whether this will be displayed on the published charts too 🙏

[bprusinowski]

Cool 💯

There are some things mentioned in the comments, mostly about using chartData instead of allData. This could result in banner disappearing and reappearing when using interactive filters, but it would reflect the reality, and only be displayed when negative values are actually used in chart 👀

Also, one nit comment: we recently started to consolidate the chart-related logic here, so you can have most of the logic in one place instead of spread out across components.

While currently the chartConfigOptionsUISpec is mostly about editor UI elements, I think it would make sense to extend the ChartSpec type with e.g. getBannerMessage, and move the negative warning logic there, so in the end it's not stored in Banner, but rather accessed through chartSpec?.getBannerMessage. What do you think?

[bprusinowski]

I think here we should use state.chartData, as the error should dynamically adjust to e.g. interactive filters (to not show it when some negative values are present in the whole dataset, but not in the current filter selection).

[bprusinowski]

I wonder here if the error shouldn't be a bit more end-user facing, like

Careful, this chart might not display correctly due to negative values.

I am not sure if we should have "Try using another chart type" displayed in published chart, what do you think?

[adintegra]

I agree with this. Also, not sure it makes sense in the published context as the user would not have control to change anything. My preference would probably not to have this messaging in the published chart. What do you think?

[bprusinowski]

I think it would make sense if it's not displayed in published chart – we have a persistent banner already if the data is imputed in area chart, but here as we are not altering the data artificially, it should be fine to not have it displayed 👍

[lloydrichards]

I would push back slightly on not showing the banner in published views. Having negative and positive numbers result in the area being distorted and visualization delivering misleading results. Public users might misinterpret the results without ever knowing that the information is misleading. This might not be the case for all banners, but for the case of negative numbers this could be exploited.

[bprusinowski]

@lloydrichards for the chart data vs all data see

visualization-tool/app/charts/shared/chart-state.ts

Lines 477 to 490 in 97a70de

	const chartData = React.useMemo(() => {
	return observations.filter(
	overEvery([
	...interactiveLegendFilters,
	...interactiveTimeRangeFilters,
	...interactiveTimeSliderFilters,
	])
	);
	}, [
	observations,
	interactiveLegendFilters,
	interactiveTimeRangeFilters,
	interactiveTimeSliderFilters,
	]);

[lloydrichards]

While currently the chartConfigOptionsUISpec is mostly about editor UI elements, I think it would make sense to extend the ChartSpec type with e.g. getBannerMessage, and move the negative warning logic there, so in the end it's not stored in Banner, but rather accessed through chartSpec?.getBannerMessage. What do you think?

I like the idea of keeping all the logic in one place, but will first need to figure out if UISpec is specific to the public chart or the configurator view. If the Banner is to share responsibility between these views then having this outside of the chartConfigOptionsUISpec makes more sense, unless there is precedence to conditionally render UI elements already?

[bprusinowski]

Right now it's scoped to the editor when it comes to the properties is has, but I think conceptually, it's related broadly to chart specifics, so now we have:

export interface ChartSpec<T extends ChartConfig = ChartConfig> {
  chartType: ChartType;
  encodings: EncodingSpec<T>[];
  interactiveFilters: InteractiveFilterType[];
}

where both encodings and interactiveFilters are used in the editor. Maybe we could have a new property there to store banner-related logic, or even restructure a bit to have a clear distinction, e.g.

export interface ChartSpec<T extends ChartConfig = ChartConfig> {
  chartType: ChartType;
  editor: {
      encodings: EncodingSpec<T>[];
      interactiveFilters: InteractiveFilterType[];
  };
  chart: {
    getBannerMessage?: ({ chartData, isPublished }: { chartData: Observation[]; isPublished: boolean }) => string
  };
}

Open to suggestions, but I think this new feature might fit nicely there 👍

[lloydrichards]

Maybe we could have a new property there to store banner-related logic, or even restructure a bit to have a clear distinction, e.g.

This is the same ChartSpec that is stored as the ChartConfig in the ConfiguratorConfig? I personally wouldnt want to misconstrue configuration and error handling, especially if this logic is being build over time. I think a more functional approach of accepting the config and returning a message depending on the view would make more sense 🤷‍♂️ like:

type Failure = ChartError | ConfigWarning | VisualizationWarning | etc

export const useConfiguratorFailureMessages = (config: ChartSpec) => Failure[]

export const useChartFailureMessages = (config: ChartSpec) => Failure[]

[bprusinowski]

I think a difference in the end would be that we have another function to access it instead of passing it as part of ChartSpec – as long as they're close together that's fine too 👌

The problem at one point was that we had such a "separation of concerns" approach to having different chunks of logic throughout the app, and it became extremely difficult to foresee all the consequences of changing something in a particular place in the app (e.g. a side effect of changing a particular component for a particular chart type); after the expansion of the high-level "chart spec" it became easier (you just look at one object and see the consolidated view of what will happen if there are missing values or a given option changes), that's why I suggested to follow this approach :)