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: mark as visited on change in addition to blur #2138

Merged
merged 3 commits into from
Feb 29, 2024

Conversation

web-padawan
Copy link
Member

Description

Fixes #2134

Type of change

  • Bugfix

Note: I'm not very happy with my approach to test this (like, I'd prefer to spy on validator). Suggestions are welcome.

Also, ideally we wouldn't need to use fireEvent manually, as native <input type="number"> fires it on Enter.
But for some reason user.type(projectInput, '123[Enter]') didn't work this way, probably RTL doesn't handle this case.

@CLAassistant
Copy link

CLAassistant commented Feb 28, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.71%. Comparing base (5d05a07) to head (0a09983).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2138   +/-   ##
=======================================
  Coverage   93.71%   93.71%           
=======================================
  Files          62       62           
  Lines        1607     1607           
  Branches      361      361           
=======================================
  Hits         1506     1506           
  Misses         66       66           
  Partials       35       35           
Flag Coverage Δ
unittests 93.71% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Lodin
Lodin previously approved these changes Feb 28, 2024
Copy link
Contributor

@Lodin Lodin left a comment

Choose a reason for hiding this comment

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

LGTM

packages/ts/react-form/test/useForm.spec.tsx Outdated Show resolved Hide resolved
sissbruecker
sissbruecker previously approved these changes Feb 28, 2024
const projectInput = await findByTestId('project');
await user.type(projectInput, '123');
// Mimic change that happens without blur
fireEvent.change(projectInput);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was looking into this a bit. I guess it doesn't work with user.type because testing library is just calling dispatchEvent for the respective keyboard events, it doesn't integrate with the browser driver to natively press keys. As such it probably can't trigger the change event of a native input.

Comment on lines +334 to +335
setCount(count + 1);
return !entity.projectId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding spy usage: It's possible to define a spy outside of the component and call the spy here. That requires adding a timeout before the assertion though, it looks like there is some async processing in the binder. Not sure if that makes things easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I guess we can keep the current approach as it's not very complicated anyway.

@web-padawan web-padawan enabled auto-merge (squash) February 29, 2024 11:26
@web-padawan web-padawan removed the request for review from Lodin February 29, 2024 11:30
@web-padawan web-padawan enabled auto-merge (squash) February 29, 2024 11:34
Copy link

sonarcloud bot commented Feb 29, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@web-padawan web-padawan merged commit 24b60af into main Feb 29, 2024
15 checks passed
@web-padawan web-padawan deleted the fix/react-change-blur branch February 29, 2024 11:42
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Hilla 24.4.0.alpha8 and is also targeting the upcoming stable 24.4.0 version.

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

Successfully merging this pull request may close these issues.

[react-form] Validators are not called for DateTimePicker
5 participants