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 #4170: Clear the previously selected date on empty input with showTimeSelectOnly #4336

Conversation

balajis-qb
Copy link

Closes #4170

Summary

This PR addresses an issue where the DatePicker was not clearing the previously selected time when the user deletes the value in the date input via keyboard. This issue was only coming when the showTimeSelectOnly is enabled.

Changes Made:

  • I updated the new date value only if the value exist, else set the null value, instead of restoring back to the previously selected value
  • I added test cases to validate the above case both with and without showTimeSelectOnly using React Test Library

…electOnly

Previously, the selected date was not being cleared and retained the previously selected time when an empty value was passed to the date input while showTimeSelectOnly was enabled due to a bug.  This commit address the issue and adds test cases to ensure proper functionality.

Closes Hacker0x01#4170
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ This pull request was sent to the PullRequest network.


@balajis-qb you can click here to see the review status or cancel the code review job.

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

PullRequest Breakdown

Reviewable lines of change

+ 56
- 13

72% JavaScript (tests)
28% JavaScript

Type of change

Fix - These changes are likely to be fixing a bug or issue.

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Merging #4336 (d009741) into main (87c8e0d) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head d009741 differs from pull request most recent head d1786ca. Consider uploading reports for the commit d1786ca to get more accurate results

@@            Coverage Diff             @@
##             main    #4336      +/-   ##
==========================================
+ Coverage   96.63%   96.67%   +0.03%     
==========================================
  Files          27       27              
  Lines        2375     2373       -2     
  Branches      953      952       -1     
==========================================
- Hits         2295     2294       -1     
+ Misses         80       79       -1     
Files Coverage Δ
src/index.jsx 94.43% <100.00%> (+0.18%) ⬆️

@martijnrusschen
Copy link
Member

Nice refactor and fix

@martijnrusschen martijnrusschen merged commit 6ff581d into Hacker0x01:main Oct 23, 2023
4 checks passed
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Overall I don't see an issue with this pull request. I note that the date !== null case ended up executing code that should have effectively been a no-op on the date value. Though I note the biggest functional change is that setSelection is no longer called for this case.

Image of David K David K


Reviewed with ❤️ by PullRequest

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.

DatePicker with "showTimeSelectOnly" is not possible clear the value
2 participants