Add GGD measurements #338

VWSCoronaDashboard4 · 2020-09-16T14:22:21Z

Summary & Motivation

Adds GGD measurements to the UI.

Detailed design

Most things are similar to the other tiles.

Is there any feedback on the way I render the KPI sentence around the percentage?

Drawbacks

n/a

Alternatives

n/a

Unresolved questions

n/a

Governance

Documentation is added
Test cases are added or updated
I've read the contributing document https://github.com/minvws/.github/blob/master/CONTRIBUTING.md
I've read and understand the Code of Conduct https://github.com/minvws/.github/blob/master/CODE_OF_CONDUCT.md
I understand that any contributions or suggestions I made may make it into the actual code. I've read the License
https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/LICENSE.txt
and the contributor license agreement https://github.com/minvws/nl-covid19-notification-app-coordination/blob/master/CLA.md

VWSCoronaDashboard1 · 2020-09-16T15:26:03Z

src/pages/landelijk/positief-geteste-mensen.tsx

+                    __html: replaceVariablesInText(
+                      text.percentage_ggd_positief_getest_getest_week_uitleg,
+                      {
+                        percentage: `<span class="text-light-blue inline-kpi">${formatDecimal(
+                          percentageDataGGD?.percentage_infected_ggd
+                        )}%</span>`,
+                        totaal: `<span class="text-dark-blue inline-kpi">${formatDecimal(
+                          percentageDataGGD?.total_tested_ggd
+                        )}</span>`,


Indeed this looks very verbose in order to just replace a few values. I wouldn't know of a cleaner way to do this though, but this is something we might need to discuss and see if we can come up with a nicer solution. For now, it works, so let's leave it.

VWSCoronaDashboard · 2020-09-16T18:23:43Z

src/locale/nl.json

+    "percentage_ggd_titel": "Aantal geteste personen via GGD",
+    "percentage_ggd_datums": "Waarde van {{dateOfReport}}. Verkregen: {{dateOfInsertion}}. Wordt dagelijks bijgewerkt.",
+    "percentage_ggd_bron": {
+      "href": "https://coronadashboard.rijksoverheid.nl/#ggd",
+      "text": "GGD"
+    },
+    "percentage_ggd_toelichting": "Dit getal zegt iets over het totaal aantal personen dat via de GGD getest is, en hoeveel daarvan een positieve uitslag hadden. Let op dat de meeste testen via de GGD lopen, maar dit niet voor alle testen geldt. Daarom wijkt het absolute aantal positief geteste personen af van het aantal positief geteste personen door de GGD.",
+    "percentage_ggd_totaal_getest_week_titel": "Aantal geteste personen via GGD",
+    "percentage_ggd_totaal_getest_week_uitleg": "Aantal personen die in één week via de GGD getest zijn.",
+    "percentage_ggd_positief_getest_week_titel": "Positief getest personen via GGD",
+    "percentage_ggd_positief_getest_week_uitleg": "Aantal en percentage positief geteste personen die in één week via de GGD getest zijn.",
+    "percentage_ggd_positief_getest_getest_week_uitleg": "dit is {{percentage}} van {{totaal}} geteste personen"


nit: Might be easier to put this all in one object, to keep them contained/namespaced.

VWSCoronaDashboard · 2020-09-16T18:24:00Z

src/pages/landelijk/positief-geteste-mensen.tsx

@@ -1,4 +1,4 @@
-import { useState } from 'react';
+import { useState, Fragment } from 'react';


No need for Fragment, you can use <>

No, that's not possible for the way it is used.

…ashboard into feature/percentage-positive

VWSCoronaDashboard8 · 2020-09-23T13:14:32Z

src/pages/landelijk/positief-geteste-mensen.tsx


 const text: typeof siteText.positief_geteste_personen =
  siteText.positief_geteste_personen;

+const percentageGgdText: typeof siteText.positief_geteste_personen_ggd =


This looks weird. Using typeof on a value to extract a type, and then assigning the same value to the type. Typescript should infer the same time if you leave out the type definition @VWSCoronaDashboard4

Looks like it happened before in this file, so maybe that inspired you...

This is legacy code, the typing is no longer necessary since the entire locale struct is typed in locale/index.ts.
So import siteText from 'locale'; already returns a fully typed object.

VWSCoronaDashboard8 · 2020-09-23T13:16:44Z

src/pages/landelijk/positief-geteste-mensen.tsx

+        <>
+          <ContentHeader
+            category={'\u00A0'}
+            title={percentageGgdText.titel}


There's a lot of percentageGgdText. type of prefixes in this file. I would destructure it first.

I've adjusted this variable name, but destructing it first causes other troubles (and it's not consistent with our use of text. in other pages)

VWSCoronaDashboard8 · 2020-09-23T13:18:15Z

src/utils/replaceKpisInText.ts

+ */
+
+const replaceKpisInText = (
+  translation?: string | undefined | null,


Why undefined and null? Also undefined is already implied by the ?

In what situation would you want to call "replace in text" without passing an actual text? Similarly why would you call this to replace kpis in a string if you do not have any kpis? In other words, are you sure these function arguments need to be optional?

VWSCoronaDashboard8 · 2020-09-23T13:20:34Z

src/utils/replaceKpisInText.ts

+ * - If no translation is given, an empty string will be returned.
+ *
+ * @param translation - Translation string with curly brackets for variables.
+ * @param kpis - An object with keys representing kpi object containing a value and class name available for replacement.


These king of annotations are a little redundant. Typescript will tell you the shape of the kpis. So now you have to remember to update the comments whenever the code changes.

VWSCoronaDashboard8 · 2020-09-23T13:25:22Z

src/utils/replaceKpisInText.ts

+  translation?: string | undefined | null,
+  kpis?: Kpi[]
+): string => {
+  const variables: { [key: string]: string | undefined } = {};


I don't see any code below that would make the result undefined. Looks like all iterations will produce a string.

…ashboard into feature/percentage-positive # Conflicts: # src/pages/landelijk/positief-geteste-mensen.tsx

VWSCoronaDashboard · 2020-09-25T11:50:39Z

In this situation I would prefer a different message

would be better as:

"Geen van de GGD testen was positief"

VWSCoronaDashboard

Please add tests for your new utility method.

Besides that, I think we should start thinking about different i18n and l10n solutions to make maintenance of this project a bit easier. I know bundle size is a concern, but still...

VWSCoronaDashboard · 2020-09-25T11:27:36Z

src/pages/landelijk/positief-geteste-mensen.tsx

+            category={'\u00A0'}
+            title={ggdText.titel}
+            id="ggd"
+            Icon={Fragment}


What's the theory behind this?

Is Icon a required prop and we just want to render nothing? Shouldn't we make Icon an optional prop?

I've never seen this pattern before and am curious about this.

Now that I look at this again... I'll work on simplifying this! It's working around the problem that without the icon it messes up the spacing of the layout, which is a silly excuse.

VWSCoronaDashboard · 2020-09-25T11:28:23Z

src/utils/replaceKpisInText.ts

+ * - If no translation is given, an empty string will be returned.
+ */
+
+export function replaceKpisInText(translation: string, kpis: Kpi[]): string {


Can you add tests for this in Jest?

VWSCoronaDashboard · 2020-09-25T11:58:48Z

src/pages/landelijk/positief-geteste-mensen.tsx

+                    __html: replaceKpisInText(
+                      ggdText.positief_getest_getest_week_uitleg,
+                      [
+                        {
+                          name: 'numerator',
+                          value: formatNumber(ggdData?.infected_ggd),
+                          className: 'text-light-blue',
+                        },
+                        {
+                          name: 'denominator',
+                          value: formatNumber(ggdData?.total_tested_ggd),
+                          className: 'text-dark-blue',
+                        },
+                      ]
+                    ),
+                  }}


This is starting to get relatively complex.
Is there a way where we can rely on the ICU message format without adding too much bloat to the bundle?

We can take a look at react-intl, or at a lower-level intl-messageformat.

https://formatjs.io/docs/core-concepts/icu-syntax#plural-format

Not sure if any of these packages would solve / improve any of the attached code. What specific part do you want to see to be simplified?

It would make pluralization easier, and it would remove the need for our custom replacement methods.

But we can take that off the table and put it on the tech debt backlog!

VWSCoronaDashboard4 · 2020-09-25T13:16:13Z

@VWSCoronaDashboard The -% would be a data issue, I've added an extra safeguard to prevent this from getting displayed.

VWSCoronaDashboard · 2020-09-25T13:19:12Z

src/components/layout/Content.tsx

@@ -35,4 +42,7 @@ interface IContentHeaderProps {
      text: string;
    };
  };
+  category?: string;
+  Icon?: React.ComponentType;


…ashboard into feature/percentage-positive # Conflicts: # src/pages/landelijk/positief-geteste-mensen.tsx

Add GGD measurements

94d8763

VWSCoronaDashboard4 requested review from VWSCoronaDashboard1 and VWSCoronaDashboard5 September 16, 2020 14:22

tkon99 mentioned this pull request Sep 16, 2020

Suggestion: add additional GGD testing measurements #339

Closed

3 tasks

VWSCoronaDashboard1 approved these changes Sep 16, 2020

View reviewed changes

VWSCoronaDashboard reviewed Sep 16, 2020

View reviewed changes

VWSCoronaDashboard added 2 commits September 17, 2020 10:35

Merge branch 'develop' of https://github.com/minvws/nl-covid19-data-d…

bc9bda1

…ashboard into feature/percentage-positive

Review comments

b7487a1

VWSCoronaDashboard1 approved these changes Sep 17, 2020

View reviewed changes

VWSCoronaDashboard4 added the Hold This should not be merged (yet) label Sep 17, 2020

VWSCoronaDashboard8 suggested changes Sep 23, 2020

View reviewed changes

VWSCoronaDashboard added 6 commits September 24, 2020 17:16

Merge branch 'develop' of https://github.com/minvws/nl-covid19-data-d…

a7cc10b

…ashboard into feature/percentage-positive # Conflicts: # src/pages/landelijk/positief-geteste-mensen.tsx

Adjustments to GGD layout

74a525d

Type issues

e20f88e

Review comments

3c2b078

PR feedback replace kpis in text utility

5b623a2

Adjusted variable names

e7a7491

VWSCoronaDashboard4 removed the Hold This should not be merged (yet) label Sep 25, 2020

VWSCoronaDashboard reviewed Sep 25, 2020

View reviewed changes

VWSCoronaDashboard added 2 commits September 25, 2020 14:46

Add unit tests for replace kpis in text

bb4f1a1

Adjust content header

5c740ba

VWSCoronaDashboard reviewed Sep 25, 2020

View reviewed changes

VWSCoronaDashboard approved these changes Sep 25, 2020

View reviewed changes

Merge branch 'develop' of https://github.com/minvws/nl-covid19-data-d…

6d87edb

…ashboard into feature/percentage-positive # Conflicts: # src/pages/landelijk/positief-geteste-mensen.tsx

VWSCoronaDashboard4 merged commit 0d953b1 into develop Sep 25, 2020

VWSCoronaDashboard4 deleted the feature/percentage-positive branch September 25, 2020 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GGD measurements #338

Add GGD measurements #338

VWSCoronaDashboard4 commented Sep 16, 2020

VWSCoronaDashboard1 Sep 16, 2020

VWSCoronaDashboard Sep 16, 2020

VWSCoronaDashboard Sep 16, 2020

VWSCoronaDashboard4 Sep 17, 2020

VWSCoronaDashboard8 Sep 23, 2020

VWSCoronaDashboard1 Sep 23, 2020 •

edited

VWSCoronaDashboard8 Sep 23, 2020

VWSCoronaDashboard4 Sep 25, 2020

VWSCoronaDashboard8 Sep 23, 2020

VWSCoronaDashboard8 Sep 23, 2020

VWSCoronaDashboard8 Sep 23, 2020

VWSCoronaDashboard8 Sep 23, 2020

VWSCoronaDashboard commented Sep 25, 2020

VWSCoronaDashboard left a comment

VWSCoronaDashboard Sep 25, 2020

VWSCoronaDashboard4 Sep 25, 2020

VWSCoronaDashboard Sep 25, 2020

VWSCoronaDashboard Sep 25, 2020

VWSCoronaDashboard4 Sep 25, 2020

VWSCoronaDashboard Sep 25, 2020

VWSCoronaDashboard Sep 25, 2020

VWSCoronaDashboard4 commented Sep 25, 2020

VWSCoronaDashboard Sep 25, 2020

		@@ -1,4 +1,4 @@
		import { useState } from 'react';
		import { useState, Fragment } from 'react';

Add GGD measurements #338

Add GGD measurements #338

Conversation

VWSCoronaDashboard4 commented Sep 16, 2020

Summary & Motivation

Detailed design

Drawbacks

Alternatives

Unresolved questions

Governance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VWSCoronaDashboard1 Sep 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VWSCoronaDashboard commented Sep 25, 2020

VWSCoronaDashboard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VWSCoronaDashboard4 commented Sep 25, 2020

Choose a reason for hiding this comment

VWSCoronaDashboard1 Sep 23, 2020 •

edited