#336/Fix broken unit tests caused by react-children-utilities dependency. #352

abrie · 2020-04-01T18:26:05Z

Related issues and PRs

Description

Fixes broken unit tests caused by react-children-utilities dependency.

FormHelpButton uses the label attribute to populate the help box. If the label is a React node then the text is difficult to access programmatically. #336 introduces a dependency, react-children-utilities for the 'onlyText' method to extract the text from React nodes. But the dependency breaks the unit tests because the ESM module format is not supported by Jest (see open issue jestjs/jest#4842).

This PR creates a HelpProps interface to communicate the help label and text to the FormHelpButton. This solution is slightly more verbose than extracting text from the 'label' attribute. But it is more explicit and breaks no existing behavior.

Impacted Areas in the application

This changes a lot of files, but in a predictable and uniform way.

Both the UI and unit tests are affected by these changes.

Testing

yarn test

This removes the react-children-utilities dependency, which fixes the tests without requiring module transpilation. The FormHelpButton uses the label field to label the help box, so if the label is a React Node then the text is difficult to access programmatically. The previous implementation used a 3rd party dependency, react-children-utilities, to extract the text. But the dependency breaks the unit tests because the module format is not supported by Jest. This commit creates a HelpProps interface to communicate help label and text to the FormHelpButton. This is more explicit, and avoids the need for another dependency.

vercel · 2020-04-01T18:26:11Z

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/covid19-scenarios/covid19-scenarios/6geanctzf
✅ Preview: https://covid19-scenar-git-fork-abrie-remove-react-children-util-7d3d32.covid19-scenarios.now.sh

abrie · 2020-04-01T18:47:05Z

package.json

@@ -82,7 +82,6 @@
    "prop-types": "15.7.2",
    "raf": "3.4.1",
    "react": "16.13.0",
-    "react-children-utilities": "2.0.12",


Remove dependency.

abrie · 2020-04-01T18:51:19Z

src/components/Form/FormHelpButton.tsx

+export interface HelpProps {
+  label: string
+  text: string
+}


This interface is passed into the 'help' attribute for elements that need a help button. Having label in this interface removes the need to try and extract label text from the the 'label' attribute on the element.

abrie · 2020-04-01T18:51:47Z

src/components/Form/FormHelpButton.tsx

+export function help(label:string = "", text:string = "") : HelpProps {
+  return { label, text }
+}
+


This is a helper function to create the HelpProps object.

abrie · 2020-04-01T18:53:15Z

src/components/Form/FormHelpButton.tsx

-            {label && <h4>{onlyText(label)}</h4>}
-            <p>{help}</p>
+            <h4>{help.label}</h4>


Now we no longer need to invoke onlyText. We just insert the contents of the 'label' field from the HelpProps object.

abrie · 2020-04-01T18:55:14Z

src/components/Main/Results/ResultsCard.tsx

@@ -106,7 +107,7 @@ function ResultsCardFunction({
            {t('Results')}
          </h2>
        }
-        help={t('This section contains simulation results')}
+        help={help(t('Results'), t('This section contains simulation results'))}


This pattern is repeated throughout all elements that have a 'help' attribute. The attribute receives a HelpParams object, created by the help(title, text) helper.

abrie · 2020-04-01T18:58:29Z

src/components/Main/Scenario/ScenarioCard.tsx

      label={<h2 className="p-0 m-0 d-inline text-truncate">{t('Scenario')}</h2>}
-      help={t('Combination of population, epidemiology, and mitigation scenarios')}
+      help={help(t('Scenario'), t('Combination of population, epidemiology, and mitigation scenarios'))}


Here is an example of a label that renders a React node. Note that we pass both a label and help text to the help() method, which generates a HelpParams object. The label is duplicated in both the label= attribute and the help() function.

gj262 · 2020-04-01T21:58:33Z

src/components/Form/FormHelpButton.tsx

@@ -12,13 +10,21 @@ function safeId(id: string) {
  return id.replace('.', '-')
 }

+export interface HelpProps {
+  label: string
+  text: string


Hi @abrie could text be changed to content and be string | React.ReactNode? When I changed this last, @ivan-aksamentov mentioned that he wanted to allow the content of help possibly be more than plain text.

@gj262 Yeah, sounds reasonable. PR Updated.

See neherlab#352 (comment)

abrie · 2020-04-01T22:39:07Z

src/components/Form/FormHelpButton.test.tsx

+  it('it displays a ReactNode', async () => {
+    const { getByLabelText, findByText, queryByText } = render(
+      <FormHelpButton identifier="abc" help={help('def', <span>Hello, ReactNode</span>)} />,
+    )
+
+    fireEvent.click(getByLabelText('help'))
+
+    await findByText('Hello, ReactNode')
+    expect(queryByText('Hello, ReactNode')).toBeTruthy()
+  })
+


@gj262 Here's a unit test for setting HelpParams content to a ReactNode

abrie · 2020-04-01T22:39:53Z

src/components/Form/FormHelpButton.tsx

+  content: string | React.ReactNode
 }

-export function help(label = '', text = ''): HelpProps {
-  return { label, text }
+export function help(label: string, content: string | React.ReactNode): HelpProps {
+  return { label, content }


@gj262 text renamed to content, and it allows React.ReactNode.

gj262 · 2020-04-01T23:59:33Z

Hi @abrie LGTM and thanks for tidying this up and fixing the test. However it looks like you missed one, the help on the Severity Card is not rendering with the preview.

abrie · 2020-04-02T00:24:32Z

@gj262 Good catch! Thanks. Updated PR.

Hi @abrie LGTM and thanks for tidying this up and fixing the test. However it looks like you missed one, the help on the Severity Card is not rendering with the preview.

ivan-aksamentov · 2020-04-02T01:36:42Z

@abrie I would prefer to just transpile the package. In fact I already did it on another branch. Will backport right now.

abrie · 2020-04-02T01:43:48Z

Ok

ivan-aksamentov · 2020-04-02T02:55:54Z

@abrie @gj262
The "correct" fix, THE ONE AND ONLY, :DDD
is here
0100bfa
this fixes the transpilation of the module and nothing else.

Now, let's talk about the refactoring I see in the diff. Adding a help() wrapper is not entirely what I thought it will be. The idea was that we can put an arbitrary component inside the help box, including links, images, list of reference materials etc. So that it would be more like the educative material than just a help box. Naturally, each box might be a completely different component, but the top-level layout should probably stay the same.

@rneher Richard has never actually used any of this functionality, but I hope we can do it later still. We have so many questions because people simply don't understand what parameters do, or whether one include another or whether they are cumulative etc. A little link to FAQ + an SVG image would do magic there.

What do you think folks?

Tremendous push would be to scaffold a bunch of components somewhere, preferably in .mdx format and move all the help text over there, so that Richard could then expand on it without being constrained by the inline componentry thing we have now.

The opposite move would be to remove this functionality, make it a string and don't bother with the package.

@abrie I only glanced over your PR, so let me know if I am absolutely wrong. It can easily be the case. In which case I trust Gavin @gj262 to merge this or something similar.

Feel free to create a coherent issue(s) to make it more transparent for the community.

gj262 · 2020-04-02T15:03:49Z

@ivan-aksamentov sounds good, your fix is similar to the one I tried in the window.open() fix. #334.

And I like @abrie's approach of making the help popover label explicit and separate from the form label as it should be IMHO (separate concerns, etc.). So if the work in this PR can be resurrected I'll happily review and merge :)

abrie · 2020-04-03T00:54:00Z

This was a poorly titled PR.

As @ivan-aksamentov noted, it is primarily a refactoring.

It fixed the unit tests as a side effect.

If the plan is a more elaborate help system, then these changes are probably worth exploring. The existing implementation is magical and could use a more explicit approach, i.e. separate concerns, etc (thanks @gj262).

For future enhancements, this PR provides an interface, HelpProps, which would be the starting point for extension with SVG, mdx, etc: #352 (comment).

abrie added 2 commits April 1, 2020 14:12

Remove react-children-utilities dependency.

0bc1add

vercel bot deployed to Preview April 1, 2020 18:26 View deployment

Apply yarn lint:fix.

2ded2c9

vercel bot deployed to Preview April 1, 2020 18:33 View deployment

abrie changed the title ~~Remove react children utilities dependency~~ #336/Fix broken unit tests caused by react-children-utilities dependency. Apr 1, 2020

abrie commented Apr 1, 2020

View reviewed changes

abrie marked this pull request as ready for review April 1, 2020 19:00

Merge remote-tracking branch 'upstream/master'

9a9794c

vercel bot deployed to Preview April 1, 2020 20:58 View deployment

gj262 reviewed Apr 1, 2020

View reviewed changes

Rename HelpParams 'text' -> 'content' and allow ReactNode type.

aff2ba4

See neherlab#352 (comment)

vercel bot deployed to Preview April 1, 2020 22:26 View deployment

Add missing content param to help helper.

45727eb

vercel bot deployed to Preview April 1, 2020 22:33 View deployment

abrie commented Apr 1, 2020

View reviewed changes

Use help helper in SeverityCard help attribute.

aef1621

vercel bot deployed to Preview April 2, 2020 00:13 View deployment

Add missing 'content' parameter to help helper.

0b8bf77

vercel bot deployed to Preview April 2, 2020 00:19 View deployment

Handle undefined help attribute by showing message.

44676e4

vercel bot had a problem deploying to Preview April 2, 2020 00:32 Failure

Fix broken code typo.

547521b

vercel bot deployed to Preview April 2, 2020 00:34 View deployment

Remove unused FormHelpButton label attribute.

30fc208

vercel bot deployed to Preview April 2, 2020 00:57 View deployment

eslint --fix

d2bdeaa

vercel bot deployed to Preview April 2, 2020 01:27 View deployment

ivan-aksamentov closed this Apr 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#336/Fix broken unit tests caused by react-children-utilities dependency. #352

#336/Fix broken unit tests caused by react-children-utilities dependency. #352

abrie commented Apr 1, 2020 •

edited

Loading

vercel bot commented Apr 1, 2020 •

edited

Loading

abrie Apr 1, 2020

abrie Apr 1, 2020

abrie Apr 1, 2020

abrie Apr 1, 2020

abrie Apr 1, 2020

abrie Apr 1, 2020

gj262 Apr 1, 2020

abrie Apr 1, 2020

abrie Apr 1, 2020

abrie Apr 1, 2020

gj262 commented Apr 1, 2020

abrie commented Apr 2, 2020

ivan-aksamentov commented Apr 2, 2020

abrie commented Apr 2, 2020

ivan-aksamentov commented Apr 2, 2020 •

edited

Loading

gj262 commented Apr 2, 2020

abrie commented Apr 3, 2020

#336/Fix broken unit tests caused by react-children-utilities dependency. #352

#336/Fix broken unit tests caused by react-children-utilities dependency. #352

Conversation

abrie commented Apr 1, 2020 • edited Loading

Related issues and PRs

Description

Impacted Areas in the application

Testing

vercel bot commented Apr 1, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gj262 commented Apr 1, 2020

abrie commented Apr 2, 2020

ivan-aksamentov commented Apr 2, 2020

abrie commented Apr 2, 2020

ivan-aksamentov commented Apr 2, 2020 • edited Loading

gj262 commented Apr 2, 2020

abrie commented Apr 3, 2020

abrie commented Apr 1, 2020 •

edited

Loading

vercel bot commented Apr 1, 2020 •

edited

Loading

ivan-aksamentov commented Apr 2, 2020 •

edited

Loading