refactor(components): :recycle: 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[bot]]

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

[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]

@lloydrichards for the chart data vs all data see https://github.com/visualize-admin/visualization-tool/blob/97a70de080ebe42c4c1ff22509dc0c64ab4cc19f/app/charts/shared/chart-state.ts#L477-L490

[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 :)