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

fix(date): update onChange to handle null value when date input is cleared #1125

Closed

Conversation

ElammariYoussef
Copy link

Related issue

Reference to the issue

#1002

Description of the issue

The date input call the onChange prop even when the "clear" native behavior is clicked. The component will send a null value.

Important

Before creating a pull request run unit tests

$ npm test

# watch for changes
$ npm test -- --watch

# For a specific file (e.g., in packages/context/__tests__/command.test.js)
$ npm test -- --watch packages/action

@@ -9,7 +9,7 @@ type Props = Omit<ComponentPropsWithRef<'input'>, 'value'> & {
value?: Date;
};

const Date = forwardRef<HTMLInputElement, Props>(
const CustomDate = forwardRef<HTMLInputElement, Props>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: je pense que tu peux laisser <Date />
Quand tu l'importeras, si tu es embêté avec un new Date, tu peux l'importer comme ça :
import { Date as DateComponent } from '.....'

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines 11 to 13
const inputElement = getByDisplayValue('2024-03-04');

expect(inputElement).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

Vu que tu fais un getBy..., si l'élément n'existe pas il va retourner une erreur.
Tu n'as donc pas besoin de faire expect(inputElement).toBeInTheDocument();

Copy link
Author

Choose a reason for hiding this comment

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

Effectivement, du coup j'ai enlevé ce test

);
const inputElement = getByLabelText('Date:') as HTMLInputElement;

fireEvent.change(inputElement, { target: { value: '2024-03-05' } });
Copy link
Contributor

Choose a reason for hiding this comment

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

C'est mieux d'utiliser userEvent, ça rapproche plus d'un comportement utilisateur

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines 63 to 65
const inputElement = getByLabelText('Date:') as HTMLInputElement;

expect(inputElement).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

Vu que tu fais un getBy..., si l'élément n'existe pas il va retourner une erreur.
Tu n'as donc pas besoin de faire expect(inputElement).toBeInTheDocument();

Copy link
Author

Choose a reason for hiding this comment

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

Effectivement, du coup j'ai enlevé ce test


expect(inputElement).toBeInTheDocument();

fireEvent.change(inputElement, {
Copy link
Contributor

Choose a reason for hiding this comment

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

C'est mieux d'utiliser userEvent, ça rapproche plus d'un comportement utilisateur

Copy link
Author

Choose a reason for hiding this comment

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

Done

Comment on lines 19 to 20
const labelElement = getByLabelText('Date début');
expect(labelElement).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

Vu que tu fais un getBy..., si l'élément n'existe pas il va retourner une erreur.
Tu n'as donc pas besoin de faire expect(labelElement).toBeInTheDocument();

Copy link
Author

Choose a reason for hiding this comment

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

Effectivement, du coup j'ai enlevé ce test

@MqtCorentin
Copy link
Contributor

@ElammariYoussef des news sur le build failed actuel ?

@ElammariYoussef
Copy link
Author

@ElammariYoussef des news sur le build failed actuel ?

Non, on a pas encore trouvé, c'est une erreur générale lors de la génération des storybooks dans le projet
On est entrain d'investiguer pour le moment

@MartinWeb
Copy link
Contributor

Je clôture la PR car je n'arrive plus à relancer la build, en la recréant il se peut que cela fonctionne à nouveau.

@MartinWeb MartinWeb closed this Apr 10, 2024
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.

5 participants