Skip to content
This repository has been archived by the owner on Apr 27, 2023. It is now read-only.

Infinite select component #505

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/ui/atoms/SelectField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ const SelectField = ({
/>
);
return (
<div className={`select-field${className ? ' ' + className : ''}`}>
<div id={id} className={`select-field${className ? ' ' + className : ''}`}>
<label id={`${id}-label`} className="typography__label typography__label--primary">
{labelText}
</label>
Expand Down
118 changes: 118 additions & 0 deletions src/ui/molecules/ToggleComponent.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
import React from 'react';
import { cleanup, render, RenderResult } from '@testing-library/react';
import ToggleComponent from './ToggleComponent';

describe('ToggleComponent', (): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There aren't any tests that cover interaction with the component as a user would drive it. Refer back to the work on the ComponentToggle, look at the tests there and take some time to understand what functionality they were testing and how they did it.

afterEach(cleanup);

it('should render correctly', (): void => {
expect(
(): RenderResult =>
render(
<ToggleComponent
id=""
selectId=""
openText=""
collapsedText=""
labelText=""
questionTitle=""
></ToggleComponent>,
),
).not.toThrow();
});

it('should render correctly with all props', (): void => {
expect(
(): RenderResult =>
render(
<ToggleComponent
id="Test"
selectId=""
openText="This is some open text"
collapsedText="This is some collapsed Text"
labelText="Label Text"
questionTitle="question title"
></ToggleComponent>,
),
).not.toThrow();
});

it('should render question title correctly', (): void => {
const questionTitle = 'Question title';
const { getByText } = render(
<ToggleComponent
id=""
selectId=""
openText=""
collapsedText=""
labelText=""
questionTitle={questionTitle}
></ToggleComponent>,
);
expect(getByText(questionTitle)).toBeInTheDocument();
});

it('should be empty in the primary select field', (): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test needs to be more specific, as once other problems are solved it won't be testing what it should be. How can you change it?

const { container } = render(
<ToggleComponent
id="SelectField"
selectId=""
openText=""
collapsedText=""
labelText=""
questionTitle=""
></ToggleComponent>,
);

expect(container.querySelector('.select-field__placeholder')).toBeInTheDocument();
});

it('should be empty in the secondary select field', (): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is not doing what you think it is doing.

const { container } = render(
<ToggleComponent
id="SelectFieldTwo"
selectId=""
openText=""
collapsedText=""
labelText=""
questionTitle=""
></ToggleComponent>,
);

expect(container.querySelector('.select-field__placeholder')).toBeInTheDocument();
});

it('should have a value in the primary select dropdown field', (): void => {
const { container } = render(
<ToggleComponent
id="selectField"
selectId=""
openText=""
collapsedText=""
labelText=""
questionTitle=""
values={[{ label: 'test-option', value: 'test' }]}
defaultValue={{ label: 'Argentina', value: 'Argentina' }}
></ToggleComponent>,
);

expect(container.querySelector('#selectField').textContent).toContain('Argentina');
});

it('should have a value in the secondary select dropdown field', (): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this test is not testing what you think it is.

const { container } = render(
<ToggleComponent
id="selectFieldTwo"
selectId=""
openText=""
collapsedText=""
labelText=""
questionTitle=""
values={[{ label: 'test-option', value: 'test' }]}
defaultValue={{ label: 'Canada', value: 'Canada' }}
></ToggleComponent>,
);

expect(container.querySelector('#selectFieldTwo').textContent).toContain('Canada');
});
});
40 changes: 40 additions & 0 deletions src/ui/molecules/ToggleComponent.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import React, { ReactNode } from 'react';
import { ContentToggle, SelectField } from '../atoms';

export interface Value {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is duplicated from the SelectField, so if in the future the interface needed updating the change would need to be made in 2 places. How could this be changed to ensure that there is only a single point of truth?

label: string;
value: string;
}

interface Props {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have a think, are all these properties needed? Are there any that are only internal to the component and perhaps can be set another way?

id: string;
selectId: string;
children?: ReactNode;
labelText: string;
openText: string;
collapsedText: string;
questionTitle: string;
defaultValue?: Value;
values?: Value[];
}

const ToggleComponent = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a major issue with this component, where it's missing a very key piece of functionality that's masked because of how you're representing the component in Storybook.

id,
selectId,
collapsedText,
openText,
questionTitle,
labelText,
defaultValue,
values,
}: Props): JSX.Element => {
return (
<div className="toggle-component" id={id}>
<h3>{questionTitle}</h3>
<SelectField id={selectId} labelText={labelText} defaultValue={defaultValue} values={values}></SelectField>
<ContentToggle id={id} collapsedText={collapsedText} openText={openText}></ContentToggle>
</div>
);
};

export default ToggleComponent;
1 change: 1 addition & 0 deletions src/ui/molecules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ export { default as SearchField } from './SearchField';
export { default as SelectedPeopleList } from './SelectedPeopleList';
export { default as SelectedOption } from './SelectedOption';
export { default as Toggle } from './Toggle';
export { default as ToggleComponent } from './ToggleComponent';
export { default as UploadProgress } from './UploadProgress';
36 changes: 36 additions & 0 deletions src/ui/stories/molecules/ToggleComponent.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a major issue with the approach below. Take a look at the over source files in Storybook, including the ComponentToggle and carefully compare those against the file below. See if you can spot where you've gone wrong, and why this isn't actually representing what you think it is.
Also, did you wonder why this shows under the Toggle menu rather than as a top level item in the sidebar? How can you fix that?

import { storiesOf } from '@storybook/react';
import { withKnobs } from '@storybook/addon-knobs';
import '../../../core/styles/index.scss';
import centered from '@storybook/addon-centered/react';
import { ContentToggle, SelectField } from '../../atoms';

storiesOf('ui | molecules/Toggle', module)
.addDecorator(withKnobs)
.addDecorator(centered)
.add(
'ToggleComponent',
(): JSX.Element => {
const collapsedText = '+ Add a second country of residence/affiliation';
const openText = '- Remove a second country of residence/affiliation';
const title = 'Your country of residence or professional affiliation';
const labelText = 'Secondary country of residence/affiliation';
const options = [
{ label: 'Argentina', value: 'argentina' },
{ label: 'Canada', value: 'canada' },
{ label: 'Denmark', value: 'denmark' },
{ label: 'France', value: 'france' },
{ label: 'Hungary', value: 'hungary' },
{ label: 'Peru', value: 'peru' },
];
return (
<div className="toggle-component">
<h3 className="typography__heading typography__heading--h3">{title}</h3>
<SelectField id="selectField" labelText="" values={options}></SelectField>
<ContentToggle id="ContentToggle" collapsedText={collapsedText} openText={openText}>
<SelectField id="SelectFieldTwo" labelText={labelText} values={options}></SelectField>
</ContentToggle>
</div>
);
},
);
3 changes: 3 additions & 0 deletions src/ui/styles/ToggleComponent.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.select-field {
margin-bottom: 1.5rem;
}
1 change: 1 addition & 0 deletions src/ui/styles/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
@import './SelectedPeopleList';
@import './ContentToggle';
@import './Toggle';
@import './ToggleComponent';
@import './UploadProgress';
@import './RichTextEditor';

Expand Down