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 #4268: Retain the selected date when changing the time #4282

Conversation

balajis-qb
Copy link

Closes #4268

Summary

This PR addresses an issue where the component was resetting the selected date of the component to the current date when changing the time. The fix ensures that the selected date remains unchanged when modifying the time, enhancing the user experience.

Changes Made

  • Implemented logic to preserve the selected date when updating the time.
  • Refactored the onTimeChange function to check if the prop date is null before using the current date

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

+ 51
- 1

87% JavaScript (tests)
13% JavaScript

Type of change

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

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 looks good, no changes are recommended to the current set. Nice work!

Image of Tyler O Tyler O


Reviewed with ❤️ by PullRequest

@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Merging #4282 (c1c89a7) into main (b10a14a) will increase coverage by 0.04%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##             main    #4282      +/-   ##
==========================================
+ Coverage   96.38%   96.43%   +0.04%     
==========================================
  Files          25       25              
  Lines        2352     2354       +2     
  Branches      958      960       +2     
==========================================
+ Hits         2267     2270       +3     
+ Misses         85       84       -1     
Files Coverage Δ
src/inputTime.jsx 96.15% <100.00%> (+4.48%) ⬆️

@martijnrusschen
Copy link
Member

Can you add a test to ensure the fix keeps working?

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 change looks good and from the description, I can see the bug in onTimeChange that initializes the date to current in original code and addressed by this change,

Image of Xiaoyong W Xiaoyong W


Reviewed with ❤️ by PullRequest

Copy link
Member

@martijnrusschen martijnrusschen left a comment

Choose a reason for hiding this comment

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

This change looks good, but please add a test to avoid breaking this in the future.

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 reviewed the updates made to #4282 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.

Reviewed by:

Image of Xiaoyong W Xiaoyong W

@balajis-qb
Copy link
Author

Thank you for your feedback, team. I'll add the testcases and update the PR

This commit includes two test cases for the InputTime component

1. It verifies that the component correctly triggers the onChange event when a date prop is provided. The test simulates a change in time input and checks if the expected date, based on the provided date prop and new time, is passed to the onChange function.

2. It ensures that the component gracefully handles the scenario when the date prop is missing. The test simulates a change in time input and checks if the expected date, based on the current date and new time, is passed to the onChange function when no date prop is provided.

Closes Hacker0x01#4268
@martijnrusschen
Copy link
Member

Thanks for adding the tests, please ensure the CI job is succeeding so we can get it merged.

@balajis-qb
Copy link
Author

Hi @martijnrusschen ,

The test case failed due to an existing issue there in the datepicket_test.test.js file. Please check the issue I created for more details #4283

I fixed the issue and updated this PR. Kindly review and let me know if any changes required

Copy link
Member

@martijnrusschen martijnrusschen left a comment

Choose a reason for hiding this comment

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

Thanks for the extra effort to make the tests work.

@martijnrusschen martijnrusschen merged commit 583ebe1 into Hacker0x01:main Oct 1, 2023
4 checks passed
@balajis-qb
Copy link
Author

It was my pleasure. Thank you for your kind words.

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.

Changing time is resetting date to current date
2 participants