This enhancement aims to add a holiday list as parameters or props to the react-datepicker component. #4189

sushilkundu143 · 2023-08-05T04:15:56Z

The background will be set to orange for holidays in the calendar. If a holiday is selected, its background color will change to blue. Additionally, on mouseover, a tooltip will display the details of the corresponding holiday.

pullrequest

✅ This pull request was sent to the PullRequest network.

@sushilkundu143 you can click here to see the review status or cancel the code review job.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 186
- 60

91% JavaScript
8% SCSS
1% Other

Generated lines of change

+ 1,352
- 1,308

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

pullrequest

This is looking like a great start, but I have some concerns left inline.

Graham C

Reviewed with ❤️ by PullRequest

pullrequest · 2023-08-05T10:14:49Z

docs-site/src/components/Examples/index.js

@@ -526,7 +531,7 @@ export default class exampleComponents extends React.Component {
                onClick={(e) =>
                  this.handleAnchorClick(
                    e,
-                    `example-${slugify(example.title, { lower: true })}`
+                    `example-${slugify(example.title, { lower: true })}`,


This unrelated change should be reverted. Trailing commas are great, but should not be added on mass within this PR - I'd recommend sending in a separate PR if you'd like to add trailing commas into places.

🔺 Best Practices (Critical)

Graham C

pullrequest · 2023-08-05T10:14:49Z

src/calendar.jsx

@@ -54,7 +54,7 @@ const DROPDOWN_FOCUS_CLASSNAMES = [
 const isDropdownSelect = (element = {}) => {
  const classNames = (element.className || "").split(/\s+/);
  return DROPDOWN_FOCUS_CLASSNAMES.some(
-    (testClassname) => classNames.indexOf(testClassname) >= 0
+    (testClassname) => classNames.indexOf(testClassname) >= 0,


All these unrelated changes should be reverted. Trailing commas are great, but should not be added on mass within this PR - I'd recommend sending in a separate PR if you'd like to add trailing commas into places.

🔺 Best Practices (Critical)

Graham C

pullrequest · 2023-08-05T10:14:50Z

src/date_utils.js

@@ -168,7 +168,7 @@ export function safeDateFormat(date, { dateFormat, locale }) {
      formatDate(
        date,
        Array.isArray(dateFormat) ? dateFormat[0] : dateFormat,
-        locale
+        locale,


All these unrelated changes should be reverted.

🔺 Best Practices (Critical)

Graham C

pullrequest · 2023-08-05T10:14:50Z

src/date_utils.js

@@ -726,23 +726,56 @@ export function getHightLightDaysMap(
  return dateClasses;
 }

+export function getHolidaysMap(


This function needs test coverage.

🔺 Improve Test Coverage (Critical)

Graham C

pullrequest · 2023-08-05T10:14:50Z

src/date_utils.js

 ) {
  const endPeriod = Math.ceil(getYear(date) / yearItemNumber) * yearItemNumber;
  const startPeriod = endPeriod - (yearItemNumber - 1);
  return { startPeriod, endPeriod };
 }
+
+// Utility function to filter the array from the Map based on the date
+export function filterArrayByDate(date, dateMap) {


This function needs test coverage.

🔺 Improve Test Coverage (Critical)

Graham C

pullrequest · 2023-08-05T10:14:50Z

src/day.jsx

@@ -26,6 +27,7 @@ export default class Day extends React.Component {
    dayClassName: PropTypes.func,
    endDate: PropTypes.instanceOf(Date),
    highlightDates: PropTypes.instanceOf(Map),
+    holidayDates: PropTypes.instanceOf(Map),


The new logic in this file needs test coverage.

🔺 Improve Test Coverage (Critical)

Graham C

pullrequest · 2023-08-05T10:14:50Z

src/index.jsx

@@ -163,13 +164,14 @@ export default class DatePicker extends React.Component {
      PropTypes.shape({
        start: PropTypes.instanceOf(Date),
        end: PropTypes.instanceOf(Date),
-      })
+      }),


All these unrelated changes should be reverted.

🔺 Best Practices (Critical)

Graham C

pullrequest · 2023-08-05T10:14:50Z

src/index.jsx

@@ -323,6 +325,11 @@ export default class DatePicker extends React.Component {
        highlightDates: getHightLightDaysMap(this.props.highlightDates),
      });
    }
+    if (prevProps.holidayDates !== this.props.holidayDates) {


The new logic in this file needs test coverage.

🔺 Improve Test Coverage (Critical)

Graham C

pullrequest · 2023-08-05T10:14:51Z

src/month.jsx

@@ -45,7 +45,7 @@ const MONTH_NAVIGATION_HORIZONTAL_OFFSET = 1;

 function getMonthColumnsLayout(
  showFourColumnMonthYearPicker,
-  showTwoColumnMonthYearPicker
+  showTwoColumnMonthYearPicker,


All these unrelated changes should be reverted.

🔺 Best Practices (Critical)

Graham C

This issue remains. It is not the job of this PR to re-format the code, even if the formatting is bad. Another PR should be created to deal with that.

Graham C

pullrequest · 2023-08-05T10:14:51Z

src/month.jsx

@@ -319,6 +320,7 @@ export default class Month extends React.Component {
          inline={this.props.inline}
          shouldFocusDayInline={this.props.shouldFocusDayInline}
          highlightDates={this.props.highlightDates}
+          holidayDates={this.props.holidayDates}


The new logic in this file needs test coverage.

🔺 Improve Test Coverage (Critical)

Graham C

pullrequest · 2023-08-05T10:14:53Z

src/week.jsx

@@ -23,11 +23,12 @@ export default class Week extends React.Component {
      PropTypes.shape({
        start: PropTypes.instanceOf(Date),
        end: PropTypes.instanceOf(Date),
-      })
+      }),


All these unrelated changes should be reverted.

🔺 Best Practices (Critical)

Graham C

pullrequest · 2023-08-05T10:14:53Z

src/week.jsx

@@ -133,6 +134,7 @@ export default class Week extends React.Component {
            includeDates={this.props.includeDates}
            includeDateIntervals={this.props.includeDateIntervals}
            highlightDates={this.props.highlightDates}
+            holidayDates={this.props.holidayDates}


The new logic in this file needs test coverage.

🔺 Improve Test Coverage (Critical)

Graham C

…rc/calendar.jsx

pullrequest

Overall no issues are noted with this PR. Though there are some suggestions that should be though about before merging

David K

Reviewed with ❤️ by PullRequest

pullrequest · 2023-08-05T20:32:54Z

.prettierrc

@@ -0,0 +1,3 @@
+{
+    "trailingComma": "none"


Did you know that by setting this to "all" typically resolves to fewer line changes in diffs. Because when a entry is added at the end of an object definition you don't end up modifying the line above to have the comma.

🔹 Giving Information (Nice to have)

David K

pullrequest · 2023-08-05T20:32:54Z

src/date_utils.js

+
+// Utility function to filter the array from the Map based on the date
+export function filterArrayByDate(date, dateMap) {
+  const formattedDate = formatDate(date, "MM.dd.yyyy");


Would reccomend having a const DATE_FORMAT = "MM.dd.yyyy" defined and use that rather than re-using the same value. This will reduce potential problems in the future.

Since I note this is in at least 3 places in the current pull request.

🔸 Reduce Future Bugs (Important)

David K

martijnrusschen · 2023-08-06T18:30:47Z

This looks very similar to https://reactdatepicker.com/#example-highlight-dates-with-custom-class-names-and-ranges. Can you explain how this would be different and can't be solved by the example above?

martijnrusschen

Code contains a bunch of unrelated changes. After checking the code, this looks very similar to the highlight day function. Can't you achieve what you're trying to do with the existing functions? Test coverage seems to be missing too.

martijnrusschen · 2023-08-06T18:31:07Z

.prettierrc

@@ -0,0 +1,3 @@
+{


This isn't related to this PR. Please revert

Agreed, this change should not be included.

Graham C

martijnrusschen · 2023-08-06T18:31:23Z

docs-site/yarn.lock

Please revert

pullrequest

PullRequest reviewed the updates made to #4189 up until the latest commit (b4101de). No further issues were found.

Reviewed by:

David K

pullrequest

All of the issues raised in the previous reviews remain, and further issues have been introduced - detailed inline.

Graham C

Reviewed with ❤️ by PullRequest

pullrequest · 2023-08-08T22:51:52Z

docs-site/src/examples/calendarWithHolidays.js

+      renderDayContents={renderDayContents}
+    />
+  );
+};


The docs-site/yarn.lock file should not be modified by this PR, and all changes to that file should not be included in this PR.

🔺 Bug (Critical)

Graham C

pullrequest · 2023-08-08T22:51:52Z

src/day.jsx

@@ -15,6 +15,7 @@ import {
  isAfter,
  getDayOfWeekCode,
  formatDate,
+  filterArrayByDate


Missing trailing comma.

🔸 Best Practices (Important)

Graham C

pullrequest · 2023-08-08T22:51:52Z

src/index.jsx

@@ -41,7 +42,7 @@ import {
  DEFAULT_YEAR_ITEM_NUMBER,
  isSameDay,
  isMonthDisabled,
-  isYearDisabled,
+  isYearDisabled


This change should not be included.

🔺 Bug (Critical)

Graham C

pullrequest

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard.

sushil4056 added 2 commits August 5, 2023 09:31

add the functionality for holidays

ce59cb5

remove extra packages

99952b5

pullrequest bot reviewed Aug 5, 2023

View reviewed changes

sushil4056 and others added 19 commits August 5, 2023 21:06

remove added commna for docs-site/src/components/Examples/index.js, s…

7d2923b

…rc/calendar.jsx

src/calendar.jsx remove commna from this

fc256b0

add

7834aa2

remove all the trailingComma here

af88327

remove trellingcomma from docs-site/src/components/Examples/index.js

55399e8

revert all the comma exist in file

b37f3cf

remove all comma I have added

9616671

added removed comma

84d1cd9

revert comma's

87661dd

remove extra comma src/calendar.jsx

2ed26d2

revert the existing commna in src/calendar.jsx

6027d00

add udpate in src/week.jsx

4a54d54

remove extra comma date_utils.js

4f4a35a

remove 1 extra commna src/date_utils.js

5d48ed7

Delete .prettierrc

7245d63

Delete package.json

a4576b0

add file

dbc3c00

remove commas

7a1058a

remove extra comman src/index.jsx

3ea5e95

pullrequest bot reviewed Aug 5, 2023

View reviewed changes

updated prettier config set to all

b4101de

martijnrusschen requested changes Aug 6, 2023

View reviewed changes

pullrequest bot reviewed Aug 7, 2023

View reviewed changes

pullrequest bot reviewed Aug 8, 2023

View reviewed changes

martijnrusschen mentioned this pull request Aug 11, 2023

Imlemented custom date picker with holiday functionality #4200

Closed

pullrequest bot reviewed Aug 23, 2023

View reviewed changes

martijnrusschen closed this Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

This enhancement aims to add a holiday list as parameters or props to the react-datepicker component. #4189

This enhancement aims to add a holiday list as parameters or props to the react-datepicker component. #4189

sushilkundu143 commented Aug 5, 2023

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

pullrequest bot left a comment

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

pullrequest bot Aug 8, 2023

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

pullrequest bot left a comment

pullrequest bot Aug 5, 2023

pullrequest bot Aug 5, 2023

martijnrusschen commented Aug 6, 2023

martijnrusschen left a comment

martijnrusschen Aug 6, 2023

pullrequest bot Aug 8, 2023

martijnrusschen Aug 6, 2023

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot Aug 8, 2023

pullrequest bot Aug 8, 2023

pullrequest bot Aug 8, 2023

pullrequest bot left a comment

This enhancement aims to add a holiday list as parameters or props to the react-datepicker component. #4189

This enhancement aims to add a holiday list as parameters or props to the react-datepicker component. #4189

Conversation

sushilkundu143 commented Aug 5, 2023

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment • edited Loading

Choose a reason for hiding this comment

PullRequest Breakdown

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Best Practices (Critical)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Best Practices (Critical)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Best Practices (Critical)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Improve Test Coverage (Critical)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Improve Test Coverage (Critical)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Improve Test Coverage (Critical)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Best Practices (Critical)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Improve Test Coverage (Critical)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Best Practices (Critical)

pullrequest bot Aug 8, 2023

Choose a reason for hiding this comment

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Improve Test Coverage (Critical)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Best Practices (Critical)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔺 Improve Test Coverage (Critical)

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔹 Giving Information (Nice to have)

pullrequest bot Aug 5, 2023

Choose a reason for hiding this comment

🔸 Reduce Future Bugs (Important)

martijnrusschen commented Aug 6, 2023

martijnrusschen left a comment

Choose a reason for hiding this comment

martijnrusschen Aug 6, 2023

Choose a reason for hiding this comment

pullrequest bot Aug 8, 2023

Choose a reason for hiding this comment

martijnrusschen Aug 6, 2023

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Aug 8, 2023

Choose a reason for hiding this comment

🔺 Bug (Critical)

pullrequest bot Aug 8, 2023

Choose a reason for hiding this comment

🔸 Best Practices (Important)

pullrequest bot Aug 8, 2023

Choose a reason for hiding this comment

🔺 Bug (Critical)

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading