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

Conversation

discodavey
Copy link
Member

No description provided.

Copy link
Contributor

@NuclearRedeye NuclearRedeye left a comment

Choose a reason for hiding this comment

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

I can see from the code that there are some React and Component concepts that you've not yet managed to grasp. The code in the 3 main files isn't for the large part doing what you think it is doing and there is still a fair bit more development to go.
I've added some hints/pointers, but consider thoroughly looking over some of the other code components. How do they work? How are they tested? How are they represented in Storybook. Remember, Storybook is very similar in outcome to PatternLab. Think about how you would add a new atom/molecule/organism to PatternLab and apply the same thinking here.

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.

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.

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.

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.

@@ -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?

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?

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?

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?

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

Successfully merging this pull request may close these issues.

2 participants