Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+58 −0
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7920278
refactor(components): :recycle: created a Banner component for displa…
lloydrichards Nov 6, 2023
933b441
refactor(components): :recycle: memoized the warning and abstract war…
lloydrichards Nov 7, 2023
912456b
refactor(dx): :fire: remove launch.json
lloydrichards Nov 7, 2023
f867472
refactor(components): :recycle: change warning memo to exhaustive deps
lloydrichards Nov 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .vscode/launch.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
// Use IntelliSense to learn about possible attributes.
// Hover to view descriptions of existing attributes.
// For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
"version": "0.2.0",
"configurations": [
{
"type": "chrome",
"request": "launch",
"name": "Launch Chrome against localhost",
"url": "http://localhost:3000",
"webRoot": "${workspaceFolder}/app"
}
]
}
lloydrichards marked this conversation as resolved.
Show resolved Hide resolved
48 changes: 27 additions & 21 deletions app/charts/area/chart-area.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
} from "@/graphql/query-hooks";
import { useLocale } from "@/locales/use-locale";

import { Banner } from "../../components/banner";
import { ChartProps } from "../shared/ChartProps";

export const ChartAreasVisualization = ({
Expand Down Expand Up @@ -78,26 +79,31 @@ export const ChartAreas = memo((props: ChartProps<AreaConfig>) => {
const { fields, interactiveFiltersConfig } = chartConfig;

return (
<AreaChart aspectRatio={0.4} {...props}>
<ChartContainer>
<ChartSvg>
<AxisTime /> <AxisHeightLinear />
<Areas /> <AxisTimeDomain />
<InteractionHorizontal />
{interactiveFiltersConfig?.timeRange.active === true && <BrushTime />}
</ChartSvg>
<Tooltip type={fields.segment ? "multiple" : "single"} />
<Ruler />
</ChartContainer>
{fields.segment && (
<ChartControlsContainer>
<LegendColor
chartConfig={chartConfig}
symbol="square"
interactive={interactiveFiltersConfig?.legend.active}
/>
</ChartControlsContainer>
)}
</AreaChart>
<>
<AreaChart aspectRatio={0.4} {...props}>
<Banner />
<ChartContainer>
<ChartSvg>
<AxisTime /> <AxisHeightLinear />
<Areas /> <AxisTimeDomain />
<InteractionHorizontal />
{interactiveFiltersConfig?.timeRange.active === true && (
<BrushTime />
)}
</ChartSvg>
<Tooltip type={fields.segment ? "multiple" : "single"} />
<Ruler />
</ChartContainer>
{fields.segment && (
<ChartControlsContainer>
<LegendColor
chartConfig={chartConfig}
symbol="square"
interactive={interactiveFiltersConfig?.legend.active}
/>
</ChartControlsContainer>
)}
</AreaChart>
</>
lloydrichards marked this conversation as resolved.
Show resolved Hide resolved
);
});
37 changes: 37 additions & 0 deletions app/components/banner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { Trans } from "@lingui/react";
import { Box } from "@mui/material";

import { HintYellow } from "@/components/hint";

import { useChartState } from "../charts/shared/chart-state";

/**
* Banner to display errors or warnings for a chart. Each chart type can have specific warnings or errors depending on the data.
*
* @returns React.ReactNode | null
lloydrichards marked this conversation as resolved.
Show resolved Hide resolved
*/
export const Banner = () => {
const state = useChartState();

switch (state.chartType) {
case "area":
const someValueIsNegative = state.allData.some(
(d) => +(state.getY(d) || 0) < 0
);
lloydrichards marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if (someValueIsNegative)
return (
<Box sx={{ mb: 4 }}>
<HintYellow iconName="datasetError" iconSize={64}>
<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>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Collaborator Author

@lloydrichards lloydrichards Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</Trans>
</HintYellow>
</Box>
);
default:
return null;
}
};
4 changes: 4 additions & 0 deletions app/locales/de/messages.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,10 @@ msgstr "Titel"

#: app/browser/dataset-preview.tsx
#: app/components/chart-preview.tsx
#: app/components/banner.tsx
msgid "dataset.error.negative-values"
msgstr "Vorsicht, dieser Datensatz enthält negative Werte und wird möglicherweise nicht korrekt mit diesem Diagrammtyp angezeigt. <0/><1>Versuchen Sie, einen anderen Diagrammtyp zu verwenden.</1>"

#: app/components/chart-published.tsx
msgid "dataset.publicationStatus.draft.warning"
msgstr "Achtung, dieser Datensatz ist im Entwurfs-Stadium.<0/><1>Diese Grafik nicht für Berichte verwenden.</1>"
Expand Down
4 changes: 4 additions & 0 deletions app/locales/en/messages.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,10 @@ msgstr "Title"

#: app/browser/dataset-preview.tsx
#: app/components/chart-preview.tsx
#: app/components/banner.tsx
msgid "dataset.error.negative-values"
msgstr "Careful, this dataset contains negative values and might not display correctly with this chart type. <0/><1>Try using another chart type.</1>"

#: app/components/chart-published.tsx
msgid "dataset.publicationStatus.draft.warning"
msgstr "Careful, this dataset is only a draft.<0/><1>Don't use for reporting!</1>"
Expand Down
4 changes: 4 additions & 0 deletions app/locales/fr/messages.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,10 @@ msgstr "Titre"

#: app/browser/dataset-preview.tsx
#: app/components/chart-preview.tsx
#: app/components/banner.tsx
msgid "dataset.error.negative-values"
msgstr "Attention, cet ensemble de données contient des valeurs négatives et pourrait ne pas s'afficher correctement avec ce type de graphique. <0/><1>Essayez d'utiliser un autre type de graphique.</1>"

#: app/components/chart-published.tsx
msgid "dataset.publicationStatus.draft.warning"
msgstr "Attention, ce jeu de données est à l'état d'ébauche.<0/><1>Ne l'utilisez pas pour une publication!</1>"
Expand Down
4 changes: 4 additions & 0 deletions app/locales/it/messages.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,6 +998,10 @@ msgstr "Titolo"

#: app/browser/dataset-preview.tsx
#: app/components/chart-preview.tsx
#: app/components/banner.tsx
msgid "dataset.error.negative-values"
msgstr "Attenzione, questo set di dati contiene valori negativi e potrebbe non essere visualizzato correttamente con questo tipo di grafico. <0/><1>Prova a utilizzare un altro tipo di grafico.</1>"

#: app/components/chart-published.tsx
msgid "dataset.publicationStatus.draft.warning"
msgstr "Attenzione, questo set di dati è una bozza.<0/><1>Non utilizzare questo grafico per un rapporto!</1>"
Expand Down
Loading