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 #4644: Handle the visibility of the previous and the next year navigation when showQuarterYearPicker is enabled #4652

Conversation

balajis-qb
Copy link

Description

Linked issue: #4644

Problem
As mentioned in the linked issue, the QuarterYearPicker has some issue with the visibility of the next and the previous arrow buttons based on the minDate and the maxDate. That's because we are not handling the Quarter year picker separately, but handling it with the month picker logic

Changes

  • Handled the quarter-year picker separately
  • Added helpers in the date_utils.js to achieve the result
  • Verified the change by test coverage

Contribution checklist

  • I have followed the contributing guidelines.
  • I have added sufficient test coverage for my changes.
  • I have formatted my code with Prettier and checked for linting issues with ESLint for code readability.

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 for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!

What to expect from this code review:
  • Comments posted to any areas of potential concern or improvement.
  • Detailed feedback or actions needed to resolve issues that are found.
  • Turnaround times vary, but we aim to be swift.

@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

+ 114
- 1

63% JavaScript (tests)
37% JavaScript

Type of change

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

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.99%. Comparing base (85aeced) to head (0407962).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4652      +/-   ##
==========================================
+ Coverage   96.97%   96.99%   +0.02%     
==========================================
  Files          28       28              
  Lines        2608     2632      +24     
  Branches     1102     1114      +12     
==========================================
+ Hits         2529     2553      +24     
  Misses         79       79              

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

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.

Code & tests look good. Just one variable name confused me for a second. Not a big deal though.

Image of Curtis Larson Curtis Larson


Reviewed with ❤️ by PullRequest

@@ -654,6 +655,36 @@
);
}

export function quarterDisabledBefore(day, { minDate, includeDates } = {}) {
Copy link

Choose a reason for hiding this comment

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

This day variable is actually a date correct? Just slightly confusing with the naming

🔹 Clarify Intent (Nice to have)

Image of Curtis Larson Curtis Larson

Copy link
Author

Choose a reason for hiding this comment

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

updated 👍

balajis-qb pushed a commit to qburst/react-datepicker-3 that referenced this pull request Apr 1, 2024
@balajis-qb balajis-qb force-pushed the issue-4644/fix/showQuarterYearPicker-navigation-button-visibility branch from 8ac9e76 to 0407962 Compare April 1, 2024 09:45
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 #4652 up until the latest commit (0407962). No further issues were found.

Reviewed by:

Image of Curtis Larson Curtis Larson

@martijnrusschen
Copy link
Member

Looking good, thank you!

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.

2 participants