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

[@mantine/dates] AmPmInput: Fix onChange being called twice #1322

Merged
merged 1 commit into from
Apr 25, 2022
Merged

[@mantine/dates] AmPmInput: Fix onChange being called twice #1322

merged 1 commit into from
Apr 25, 2022

Conversation

nrayburn-tech
Copy link
Contributor

Fixes #1256

When entering a or p into the AmPmInput, the onChange event is triggered twice. This adds a call to event.preventDefault, so that onChange is called a single time.

@rtivital
Copy link
Member

Have you checked that with React 18? Storybook and docs are currently working with React 17 and there is not any way to check that in dev environment.

@nrayburn-tech
Copy link
Contributor Author

I manually changed the project to React 18, tested that the bug reproduced, made the code change, and tested that it didn’t reproduce. (Ignoring the type errors from the implicit children #1245.)
I used one of the TimeInput Storybooks to test. I didn’t check the docs.

I’ll look again to see if I can find any issues or a definite reason why there’s a difference between 17/18.

@nrayburn-tech
Copy link
Contributor Author

Updated so that the focus behaves the same as currently on React 17 without the change. Also added a TimeInputRange demo, so that the focus can be tested in Storybook.

As far as I can tell both React 17 and React 18 both call the onChange function prop twice. Once during keyDown and once during onChange. The difference is that when React 17 does it, the value only changes once. When React 18 does it, the value changes twice so it appears to not change at all to the user. (Goes from am to pm and then back to am.)

I don't think I'll be able to find out anything more specific as to why this happens differently now.

@rtivital rtivital merged commit 6cc79d8 into mantinedev:master Apr 25, 2022
@rtivital
Copy link
Member

Alright, thanks for PR and testing!

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.

TimeInput AM/PM React 18
2 participants