Skip to content

Fix #4170: Clear the previously selected date on empty input with showTimeSelectOnly#4336

Merged
martijnrusschen merged 1 commit intoHacker0x01:mainfrom
qburst:issue-4170/fix/clear-selected-date-on-empty-input-with-showTimeSelectOnly
Oct 23, 2023
Merged

Fix #4170: Clear the previously selected date on empty input with showTimeSelectOnly#4336
martijnrusschen merged 1 commit intoHacker0x01:mainfrom
qburst:issue-4170/fix/clear-selected-date-on-empty-input-with-showTimeSelectOnly

Conversation

@balajis-qb
Copy link
Copy Markdown
Contributor

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
Copy Markdown

@hackerone-code hackerone-code 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
Copy Markdown

@hackerone-code hackerone-code 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
Copy Markdown

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
Copy Markdown
Member

Nice refactor and fix

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

@hackerone-code hackerone-code 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