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.

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

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

Closed

Conversation

abrie
Copy link
Contributor

@abrie abrie commented Apr 1, 2020

Related issues and PRs

#336

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
Copy link

vercel bot commented Apr 1, 2020

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 abrie changed the title Remove react children utilities dependency #336/Fix broken unit tests caused by react-children-utilities dependency. Apr 1, 2020
@@ -82,7 +82,6 @@
"prop-types": "15.7.2",
"raf": "3.4.1",
"react": "16.13.0",
"react-children-utilities": "2.0.12",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove dependency.

Comment on lines 13 to 16
export interface HelpProps {
label: string
text: string
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines 18 to 21
export function help(label:string = "", text:string = "") : HelpProps {
return { label, text }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a helper function to create the HelpProps object.

Comment on lines 39 to 45
{label && <h4>{onlyText(label)}</h4>}
<p>{help}</p>
<h4>{help.label}</h4>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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'))}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines 43 to +44
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'))}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@abrie abrie marked this pull request as ready for review April 1, 2020 19:00
@@ -12,13 +10,21 @@ function safeId(id: string) {
return id.replace('.', '-')
}

export interface HelpProps {
label: string
text: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gj262 Yeah, sounds reasonable. PR Updated.

Comment on lines +40 to +50
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()
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines 15 to 19
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 }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@gj262
Copy link
Collaborator

gj262 commented Apr 1, 2020

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
Copy link
Contributor Author

abrie commented Apr 2, 2020

@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
Copy link
Member

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

@abrie
Copy link
Contributor Author

abrie commented Apr 2, 2020

Ok

@ivan-aksamentov
Copy link
Member

ivan-aksamentov commented Apr 2, 2020

@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
Copy link
Collaborator

gj262 commented Apr 2, 2020

@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
Copy link
Contributor Author

abrie commented Apr 3, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants