Marks holidays in the calendar #4203

tanmayIntelli · 2023-08-12T11:41:20Z

With this , now the consumers will be able to provide a list of holiday and its name. On the Calendar, once the user hover over the holidays, it will show an overly with the name of the holiday. Holidays will be marked with different colour background and are selectable.
While consuming the react-datepicker, the dates to be marked as holidays will be provided in YYYY-MM-DD format.

pullrequest

✅ This pull request was sent to the PullRequest network.

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

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 441
- 101

37% Markdown
33% JavaScript (tests)
24% JavaScript
6% SCSS

Type of change

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

pullrequest

This PR is well-organized and documented. It is good to see proper unit test coverage as well—that helps quite a bit. Overall, I didn't see too many major issues.

Throughout the review, I had some minor feedback. I did find one bug with the manual implementation of pad start, and it's worth looking into removing that outright in favor of String.padStart if possible.

The only larger concern is the place where we're attempting to mutate props. That should definitely be addressed.

Otherwise, this looks quite good. Thanks for the excellent PR write-up with screenshots.

Ryan

Reviewed with ❤️ by PullRequest

pullrequest · 2023-08-13T02:38:37Z

src/date_utils.js

+    return null; // Returning null to indicate no valid date provided
+  }
+
+  const dateParts = dateString.split("-");


As a quick gut check, are we sure this part of the implementation is necessary?

Could we achieve everything here with the following?

new Date(dateString)

A check for "Invalid Date"

The Date constructor is pretty good at parsing date strings like "2023-08-12" as it is. What use cases aren't covered by that?

🔹 Simplify Code (Nice to have)

Ryan

The thought here was to keep it user friendly. For Christmas day holiday, I want the user to enter date as 2023-12-25 instead of 2023-11-25.

Adding my thoughts here because I believe this would definitely get flagged in an open-source pull request review.

Generally speaking, using the Date(...) parser is preferred for dates because it generally adheres to the ISO 8601 format [1].

In rare circumstances it does not, so sometimes custom parsers are used. However most of the time, users use something like moment.js to manage ISO 8601 compliance with the dates.

I would refer to [2] for a StackOverflow discussion on the matter. My preference would be to let Date(...) handle this logic, but either way I think it should adhere as close as possible to the ISO standard (which this does not, since it would accept "2023-8-12".

Speaking strictly for myself, making something like this more flexible / user friendly may be more trouble than it's worth.

Also just to restate: If this were strictly a internal tool, this comment would matter less. But the comments at the top of the pullrequest mentioned to explicitly flag issues that open source contributors would comment on, which is why I'm mentioning it here.

[1] https://en.wikipedia.org/wiki/ISO_8601
[2] https://stackoverflow.com/questions/4829569/parsing-iso-8601-date-in-javascript

Luciano C

I understand that I am being asked to use Date() to convert the input string to date format. At the end of the function, I parse it through Date() parser before returning.
I am still unable to understand what I am missing here.

I understand that I am being asked to use Date() to convert the input string to date format. At the end of the function, I parse it through Date() parser before returning.

I think my main comment is that it's probably not worth it to have anything other than Date(...) parse the value and check whether it's valid. I understand the intention to make the logic proceeding that more user friendly, but by my read it's more likely to make it more confusing.

However if you want to leave it, it's not explicitly incorrect, just likely to get flagged in a open-source repo for review.

Luciano C

I am still unable to understand what I am missing here.

Sorry that this became confusing for you. I think your point on user friendliness is well-made, and I agree that it should be nice and simple for consumers of this component to specify the dates.

The point we're addressing here is actually more to do with how specifically we obtain a valid Date from the input.

It is more favorable to defer to an existing implementation from JS rather than implement our own manual parsing.

This reduces the code surface area, which means less to maintain and less likelihood of bugs now and in the future.

For Christmas day holiday, I want the user to enter date as 2023-12-25 instead of 2023-11-25.

I totally understand your concern here. This is actually not an issue specifically because it is a string.

Passing in arguments of type number does require you to offset the month by 1

new Date(2023, 11, 25).toDateString() // Mon Dec 25 2023'

Passing in a parseable date string does not require you to offset the month by 1

new Date("2023-12-25").toDateString(); // Mon Dec 25 2023

Ryan

I finally understand the concern now. Thanks to the explanation and the examples you shared. With using isValid from date-fns it eliminated the requirement of this function.

pullrequest · 2023-08-13T02:38:37Z

src/date_utils.js

+  const month = parseInt(dateParts[1]);
+  const day = parseInt(dateParts[2]);
+
+  if (isNaN(year) || isNaN(month) || isNaN(day)) {


As a best practice, since these are already parsed, consider Number.isNaN here.

It is a more robust version of the original, global isNaN() function.
source

🔹 Best Practices (Nice to have)

Ryan

Thanks, updated.

pullrequest · 2023-08-13T02:38:37Z

src/date_utils.js

+      const classNamesArr = dateClasses.get(key) || [];
+      if (!classNamesArr.includes(defaultClassName)) {
+        classNamesArr.push(defaultClassName);
+        classNamesArr.push(holidayDates[i].holidayName);


I see that this is called classNamesArr, but then we're also using it to hold the name of the holiday itself—which I'm assuming is entries like "Thanksgiving Day".

In that case, it doesn't really seem to be an array of class names.

More importantly, is there a reason to use this tuple format rather than just a plain object, such as:

{ className, holidayName }

This would make the code easier to read and maintain, since any developer working on it does not need to remember what's in the first or second position, or risk confusing the two.

🔹 Simplify Code (Nice to have)

Ryan

Changed the structure to { className, holidayName }

pullrequest · 2023-08-13T02:38:38Z

src/day.jsx

+    if (holidays.has(dayStr)) {
+      return [holidays.get(dayStr)[0]];
+    }
+    return;


I think this last return statement here could be removed, it is redundant.

🔹 Simplify Code (Nice to have)

Ryan

pullrequest · 2023-08-13T02:38:38Z

src/day.jsx

+  getTitle = () => {
+    const { day, holidays = new Map() } = this.props;
+
+    const strMon =


This implementation actually has a bug. Because the comparison is day.getMonth() > 9 rather than what we're actually formatting (day.getMonth() + 1), this will produce output of 010 for October.

🔸 Bug (Important)

Ryan

I am sorry but I couldn't understand what you are trying to convey.
const strMon = day.getMonth() > 9 ?${day.getMonth() + 1}:0${day.getMonth() + 1};
By the time day comes here, it is already formatted using Date(). For October, day.getMonth() will return 9. Thus, at the return we will add 1 and append a 0 to it. So it will be 10 not 010.
Please let me know if I misunderstood something.

Sorry, I couldn’t understand what you are trying to convey here.
By the time control comes to this function, the date is already formatted in Date() format. For October, the day.getMonth() will return 9. Hence, using the check here, since 9 is not greater than 9, it will return (9+1).
Let me know if I misunderstood something.

You have since replaced this code, so it is less important.

But just for your understanding, here is a reproducible example with your previous implementation:

function getCompareDt(day) { const strMon = day.getMonth() > 9 ? `${day.getMonth() + 1}` : `0${day.getMonth() + 1}`; const strDate = day.getDate() > 9 ? `${day.getDate()}` : `0${day.getDate()}`; const stryear = day.getFullYear(); return `${strMon}.${strDate}.${stryear}`; } getCompareDt(new Date("2023-10-01")) // returns '010.01.2023'

Notice that the 10 is padded with an extra 0.

The fix is to change it to:

const strMon = day.getMonth() + 1 > 9 ? `${day.getMonth() + 1}` :

to account for the offset of 1.

Ryan

pullrequest · 2023-08-13T02:38:38Z

src/day.jsx

+  getTitle = () => {
+    const { day, holidays = new Map() } = this.props;
+
+    const strMon =


Is there a reason we can't just use the String.padStart method here? That would make this manual implementation unnecessary.

const strMon = `${day.getMonth() + 1}`.padStart(2, '0'); const strDate = `${day.getDate()}`.padStart(2, '0');

🔹 Simplify Code (Nice to have)

Ryan

Thanks, this will achieve the same thing. Updated

pullrequest · 2023-08-13T02:38:38Z

src/day.jsx

+      day.getMonth() > 9 ? `${day.getMonth() + 1}` : `0${day.getMonth() + 1}`;
+    const strDate =
+      day.getDate() > 9 ? `${day.getDate()}` : `0${day.getDate()}`;
+    const stryear = day.getFullYear();


Nit: this looks like it's meant to be in camel-case as:

const strYear = day.getFullYear();

🔹 Style Consistency (Nice to have)

Ryan

pullrequest · 2023-08-13T02:38:38Z

src/day.jsx

+    const strDate =
+      day.getDate() > 9 ? `${day.getDate()}` : `0${day.getDate()}`;
+    const stryear = day.getFullYear();
+    const compareDt = `${strMon}.${strDate}.${stryear}`;


Sorry, I might be missing something. Is there a reason we can't also just use the formatDate function here?

If possible, it seems that would be simpler than needing to manually reconstruct the key when we want to retrieve from the map.

🔸 Reduce Future Bugs (Important)

Ryan

That's correct. Have updated to use formatDate

pullrequest · 2023-08-13T02:38:38Z

src/index.jsx

@@ -356,6 +359,10 @@ export default class DatePicker extends React.Component {
      : newDate();

  calcInitialState = () => {
+    // Convert the date from string format to standard Date format
+    this.props.holidays?.forEach((day) => {
+      if (day.date.length <= 10) day.date = stringToDate(day.date);


This seems like a code smell.

We shouldn't be internally mutating the values of props. This is an anti-pattern in React, which breaks unidirectional data flow.

This problem of needing to manually adjust the date strings based on length seems like something that shouldn't be necessary at this point in the code.

Perhaps there is a cleaner way to make sure we don't run into this problem to begin with. I'd recommend revisiting this approach to see if it could be avoided outright.

🔸 Reduce Future Bugs (Important)

Ryan

I agree that we shouldn't mutate value of props. Thanks for pointing this. I am using a local variable to achieve this now.

pullrequest · 2023-08-13T02:38:38Z

test/date_utils_test.js

+    });
+  });
+
+  describe("stringToDate", () => {


Nice job with adding these tests here.

◽ Compliment

Ryan

pullrequest · 2023-08-13T02:38:40Z

test/day_test.js

+      const holidays = new Map([
+        [
+          "08.15.2023",
+          ["react-datepicker__day--holidays", "India's Independence D"],


Nit: is there a reason to leave off the last 2 characters of ay for Day here?

🔹 Style Consistency (Nice to have)

Ryan

Don't know how I missed it. This was unintentional. Have fixed it now

No big deal at all! Thanks for fixing it.

Ryan

pullrequest

Overall looks good.

I left some minor comments about variable naming. Some of these may be intentional, I made sure to note this in the comments.
I agree with the other reviewer about the parsing of date values. I left some references to ISO 8601 date formatting and some of the nuances when using Date(...) to parse these values.
There were a handful of cases where unused code or un-mutated variables could be slightly optimized.
I did flag one potential issue with the use of hasOwnProperty vs use of the in operator. I left comments and links to various tradeoffs between the two. Note that this also may be intentional.

Thank you for adding exhaustive tests to the new code. That is often overlooked in open source contributions but is critically important to keeping the open source ecosystem healthy.

Luciano C

Reviewed with ❤️ by PullRequest

pullrequest · 2023-08-14T03:18:02Z

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

@@ -289,6 +290,10 @@ export default class exampleComponents extends React.Component {
      title: "Highlight dates with custom class names and ranges",
      component: HighlightDatesRanges,
    },
+    {
+      title: "Holidays dates",


Potential Typo: You may have meant "Holiday dates" rather than "Holidays dates", since the component name is "HolidayDates"

🔹 Spelling/Typo (Nice to have)

Luciano C

pullrequest · 2023-08-14T03:18:02Z

docs/datepicker.md

+| `popperClassName`            | `string`                       |                 |                                                                                                                                                                                                        |
+| `popperContainer`            | `func`                         |                 |                                                                                                                                                                                                        |
+| `popperModifiers`            | `object`                       |                 |                                                                                                                                                                                                        |
+| `popperPlacement`            | `enumpopperPlacementPositions` |                 |                                                                                                                                                                                                        |


May be a typo: enumpopperPlacementPositions to maybe enum('popperPlacement'\|'Positions') ?

🔹 Spelling/Typo (Nice to have)

Luciano C

This is an existing string. I haven't made any changes here.

pullrequest · 2023-08-14T03:18:02Z

docs/datepicker.md

+| `popperContainer`            | `func`                         |                 |                                                                                                                                                                                                        |
+| `popperModifiers`            | `object`                       |                 |                                                                                                                                                                                                        |
+| `popperPlacement`            | `enumpopperPlacementPositions` |                 |                                                                                                                                                                                                        |
+| `preventOpenOnFocus`         | `bool`                         | false           | When this is true, the datepicker will not automatically open when the date field is focussed                                                                                                          |


Typo: focussed -> focused

🔹 Spelling/Typo (Nice to have)

Luciano C

Some of these may not have been introduced by this PR. It may just be the chaotic nature of maintaining Markdown tables + Git diffs that makes these appear in the diff.

Ryan

Same as above. I did not update any existing entry.

src/date_utils.js

pullrequest · 2023-08-14T03:18:02Z

src/day.jsx

+    const dayStr = formatDate(day, "MM.dd.yyyy");
+    // Looking for className in the Map of {day string: {className, holidayName}}
+    if (holidays.has(dayStr)) {
+      return [holidays.get(dayStr).className];


Question: This appears to return an array of size 1, that includes the className. I just want to confirm that this is intentional, as the comment above the function is not explicit about returning an array.

🔹 Bug (Nice to have)

Luciano C

yes this is intentional. I have updated the comment.

Sounds good. Thanks for updating the comment 👍

Luciano C

pullrequest

I left a clarification re: ISO timestamp parsing.

Generally it is OK to leave the logic as-is, I just believe it's highly likely to get flagged in an open-source repo.

Luciano C

Reviewed with ❤️ by PullRequest

pullrequest

Posting the first series of replies

Ryan

Reviewed with ❤️ by PullRequest

pullrequest

Clarified the point with example on how to allow JS to handle the parsing.

Ryan

Reviewed with ❤️ by PullRequest

src/date_utils.js

pullrequest · 2023-08-14T13:49:16Z

src/index.jsx

@@ -356,6 +359,11 @@ export default class DatePicker extends React.Component {
      : newDate();

  calcInitialState = () => {
+    // Convert the date from string format to standard Date format
+    const modifiedHolidays = this.props.holidays?.map((day) => {
+      if (day.date.length <= 10) day.date = stringToDate(day.date);


OK, this is where we could simplify things and perhaps make stringToDate redundant.

This library is already using date-fns, which has a function called isValid. It determines whether an existing Date instance is valid or not.

import { isValid } from 'date-fns';

Rather than checking length and doing custom parsing, we could simply do the following:

const modifiedHolidays = holidays .reduce((accumulator, holiday) => { const date = new Date(holiday.date); if (!isValid(date)) { return accumulator; } return [...accumulator, { ...holiday, date }]; }, []);

The above example does a reduce (rather than a map followed by a filter).

It simply attempts to parse the date string, with which YYYY-MM-DD is valid.
Then, it uses isValid to determine if we received a valid Date instance.
If not, it filters it out with return accumulator.

Otherwise, it adds it to the list.

Example

The following input

const holidays = [ { date: "2023-11-23", holidayName: "Thanksgiving" }, { date: "2023-12-25", holidayName: "Christmas" }, { date: "asdfasdf", holidayName: "Fake Holiday 1" }, { holidayName: "Fake Holiday 2" }, {}, ];

Produces this output:

[ { date: 2023-11-23T00:00:00.000Z, holidayName: 'Thanksgiving' }, { date: 2023-12-25T00:00:00.000Z, holidayName: 'Christmas' } ]

demonstrating that only those with valid dates made it in.

🔹 Simplify Code (Nice to have)

Ryan

One last point here—the current implementation does not handle duplicates. That might be worth taking care of if that edge-case is of-interest.

Actually that raises another question—how does the current implementation behave if there is more than one holiday registered for a given date?

Ryan

Thanks for the explanation. I am using the code snippet that you provided. That removes the requirement of `stringToDate` .
To answer your question about handling more than one holiday registered for a given date, current implementation registers the first holiday's text for the overlay.

pullrequest

Added clarification on bug in previous implementation with reproducible example
Pointed out 2 potential edge cases

Ryan

Reviewed with ❤️ by PullRequest

pullrequest

PullRequest reviewed the updates made to #4203 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.

Reviewed by:

Ryan

pullrequest

Good catch with the duplicates.

Current approach looks good, and if it were implemented differently that would likely need to get thought out.

Just wanted to add that there are many use cases for duplicates and I know other calendar libraries in other languages support them, so this may get mentioned in open source reviews.

Luciano C

Reviewed with ❤️ by PullRequest

pullrequest · 2023-08-15T02:16:53Z

test/day_test.js

@@ -1024,4 +1058,56 @@ describe("Day", () => {
      });
    });
  });
+
+  describe("title", () => {


Could be useful to add a quick test for current duplicates behavior—demonstrating that it's intended.

e.g., "uses the first holiday for a given date as the title"

This is also useful if another PR later changes the feature—alerting the submitter to the fact that they'll need to update the test for the new behavior.

🔹 Improve Test Coverage (Nice to have)

Ryan

I have updated the source code since, it would be a likely scenario that a particular day can have more than one event/festival. I have added a test case covering the same.

tanmayIntelli · 2023-08-15T06:44:53Z

I had missed to handle the case where user can add multiple entry for the same date with different holiday names. I have modified the design a little to handle this case.

pullrequest

The latest improvements look quite good. I had a few notes, but I don't think I'll have much more feedback after this point, as all of the main concerns have already been addressed.

Nice work on this one, and thanks again for your patience during all of the review feedback.

Suggestion for a future PR

I had one additional point that I don't think should be addressed in this PR. This PR has already received quite a lot of feedback and changes as-is.

However, one lingering thought here is that the UX on the holiday tooltip could use some limits to it.

Especially now that more than one holiday can be in the tooltip, it is worth considering what one or two very long holiday names would produce in terms of UX.

For example, consider Indonesia's Independence Day, which in some cases could be represented as:

Hari Proklamasi Kemerdekaan Republik Indonesia

If there were other events on that day, it might degrade the UX a bit.

It is worth thinking through what that might look like in an ideal situation—perhaps just ellipses beyond a certain length.

Either way, I think it's better addressed in a separate, simpler task where that can be the focus.

Ryan

Reviewed with ❤️ by PullRequest

pullrequest · 2023-08-15T15:32:52Z

src/date_utils.js

+    return false;
+  }
+
+  for (let i = 0; i < array1.length; i++) {


Modern equivalent, if preferred:

function arraysAreEqual(array1, array2) { if (array1.length !== array2.length) { return false; } return array1.every((value, index) => value === array2[index]); }

🔹 Simplify Code (Nice to have)

Ryan

Updated to use every

src/date_utils.js

pullrequest · 2023-08-15T15:32:52Z

src/day.jsx

+    const { day, holidays = new Map() } = this.props;
+    const compareDt = formatDate(day, "MM.dd.yyyy");
+    if (holidays.has(compareDt)) {
+      return holidays.get(compareDt).holidayName


Nit: does this check still suffice?

If holidayName can be an empty array, that would mean that a truthy check would pass here for rendering the string, and this would appear as an empty string.

I suppose it's not a huge deal since the fallback case is just an empty string. But I'd think we'd want to check for array length here to do it right.

🔹 Best Practices (Nice to have)

Ryan

Updated the check.

src/day.jsx

codecov · 2023-08-16T07:02:08Z

Codecov Report

Merging #4203 (eb86965) into main (3806d9e) will decrease coverage by 0.29%.
Report is 26 commits behind head on main.
The diff coverage is 85.00%.

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

@@            Coverage Diff             @@
##             main    #4203      +/-   ##
==========================================
- Coverage   93.75%   93.46%   -0.29%     
==========================================
  Files          20       20              
  Lines        1904     1942      +38     
  Branches      464      472       +8     
==========================================
+ Hits         1785     1815      +30     
- Misses         43       48       +5     
- Partials       76       79       +3

Files Changed	Coverage Δ
src/calendar.jsx	`94.09% <ø> (ø)`
src/index.jsx	`87.95% <0.00%> (-1.17%)`	⬇️
src/month.jsx	`93.81% <ø> (ø)`
src/week.jsx	`89.65% <ø> (ø)`
src/day.jsx	`93.37% <93.75%> (-1.52%)`	⬇️
src/date_utils.js	`99.27% <100.00%> (+0.05%)`	⬆️

pullrequest · 2023-08-16T13:56:44Z

test/day_test.js

+      const shallowDay = renderDay(day, {
+        holidays: holidays,
+      });
+      console.log("TANMAY: ", shallowDay.debug());


Whoops, I think you might have forgotten to remove this

🔹 Dead Code (Nice to have)

Ryan

Thanks for that save. Removed it

pullrequest

Thank you for addressing the remaining changes.

I would echo the other reviewer's recommendations for UI changes for multiple holidays on the tooltip. I would add that tooltips are often what screen readers use for ADA compliance.

Luciano C

Reviewed with ❤️ by PullRequest

tanmayIntelli · 2023-08-17T04:05:12Z

Thank you for addressing the remaining changes.

I would echo the other reviewer's recommendations for UI changes for multiple holidays on the tooltip. I would add that tooltips are often what screen readers use for ADA compliance.

Luciano C

Reviewed with ❤️ by PullRequest

I would try to get that accomplished as part of a separate task.

tanmayIntelli · 2023-08-17T06:51:05Z

Is it good to be merged ?

pullrequest

Is it good to be merged ?

It looks like the repository has some workflow checks that have not been run yet. I believe this depends on the maintainers of this repository, so you may have to wait for them to look over this first.

Ryan

tanmayIntelli · 2023-08-21T09:41:31Z

Do I need to tag someone here to get it merged ? This is my first contribution , I am not well versed with the steps to be followed.

Zarthus

Hey,

Thanks for the contribution @tanmayIntelli - I think your MR looks good but there's a little bit of noise in the diffs yet that make it hard to review.

Could you remove the noise introduced (specifically referring to the trailing commas, such as in components/Examples/index.js:534, calendar.jsx:57, calendar.jsx:101, ... index.jsx:168, month.jsx:48, 71 and so on?)

I'm not against those changes but generally speaking it makes reviewing harder when there's that much noise, that it's best left to a separate MR to address so that the git blame lines up with that change too.

I'll be happy to check if everything works and get it merged after that, cc @martijnrusschen - do you agree with this sentiment?

Update index.js to remove trailing commas

Update calendar.jsx to remove trailing commas

Update date_utils.js to remove trailing commas

Update day.jsx to remove trailing commas

Update index.jsx to remove trailing commas

Update month.jsx to remove trailing commas

Update week.jsx to remove trailing commas

Update date_utils_test.js to remove trailing commas

Update day_test.js to remove trailing commas

tanmayIntelli · 2023-08-22T09:47:35Z

@Zarthus I have removed the trailing commas that I could. Please review. Thanks

pullrequest

Thanks again @tanmayIntelli for your patience during this review and for all of your hard work on adding this feature!

Ryan

Reviewed with ❤️ by PullRequest

tanmayIntelli · 2023-08-24T04:45:00Z

Thanks again @tanmayIntelli for your patience during this review and for all of your hard work on adding this feature!

Ryan

Reviewed with ❤️ by PullRequest

Thanks Ryan and Luciano for review and great suggestions. Appreciate it.

Initial code changes to add holidays to datePicker

a6bb275

pullrequest bot reviewed Aug 12, 2023

View reviewed changes

pullrequest bot reviewed Aug 13, 2023

View reviewed changes

tanmayIntelli added 2 commits August 13, 2023 12:27

addressed review comments

fa93539

addressed review comments

8f59c78

pullrequest bot reviewed Aug 14, 2023

View reviewed changes

addressed review comments

17a9b80

pullrequest bot reviewed Aug 14, 2023

View reviewed changes

Addressing review comments

b01667a

pullrequest bot reviewed Aug 14, 2023

View reviewed changes

pullrequest bot reviewed Aug 15, 2023

View reviewed changes

handling a case where multople holiday name are added for same date

eeb6fc6

Added holiday in example

7a25cb3

pullrequest bot reviewed Aug 15, 2023

View reviewed changes

addressed review comments

7fe8386

trying to increase code coverage

1ffc924

pullrequest bot reviewed Aug 16, 2023

View reviewed changes

Removed console log

f24f34f

pullrequest bot reviewed Aug 16, 2023

View reviewed changes

pullrequest bot reviewed Aug 17, 2023

View reviewed changes

Zarthus self-assigned this Aug 22, 2023

Zarthus self-requested a review August 22, 2023 07:39

Zarthus assigned tanmayIntelli and unassigned Zarthus Aug 22, 2023

Zarthus suggested changes Aug 22, 2023

View reviewed changes

Zarthus mentioned this pull request Aug 22, 2023

added holiday calendar to examples #4213

Closed

tanmayIntelli added 13 commits August 22, 2023 14:39

removed unnecessary commas

a76b957

removing extra comma

94541fc

removed unnecessary commas

0368932

removed unnecessary commas

f15a608

Update index.js

cf498a3

Update index.js to remove trailing commas

Update calendar.jsx

494e1c7

Update calendar.jsx to remove trailing commas

Update date_utils.js

1aa48c0

Update date_utils.js to remove trailing commas

Update day.jsx

4d2511c

Update day.jsx to remove trailing commas

Update index.jsx

b64bc91

Update index.jsx to remove trailing commas

Update month.jsx

d5dccee

Update month.jsx to remove trailing commas

Update week.jsx

beb09b0

Update week.jsx to remove trailing commas

Update date_utils_test.js

778d182

Update date_utils_test.js to remove trailing commas

Update day_test.js

6502b4a

Update day_test.js to remove trailing commas

tanmayIntelli requested a review from Zarthus August 22, 2023 10:20

Zarthus approved these changes Aug 22, 2023

View reviewed changes

pullrequest bot reviewed Aug 24, 2023

View reviewed changes

Zarthus merged commit 738c218 into Hacker0x01:main Aug 24, 2023
4 checks passed

snyk-io bot mentioned this pull request Jun 11, 2024

[Snyk] Upgrade react-datepicker from 4.11.0 to 4.25.0 Opetushallitus/rekisterointi#30

Closed

jones58 mentioned this pull request Aug 6, 2024

[Snyk] Upgrade react-datepicker from 4.8.0 to 4.25.0 jones58/personal-knowledge-wiki#29

Merged

rowidanagah mentioned this pull request Aug 25, 2024

[Snyk] Upgrade react-datepicker from 4.11.0 to 4.25.0 rowidanagah/EduMentorMate#9

Open

Marks holidays in the calendar #4203

Marks holidays in the calendar #4203

Conversation

tanmayIntelli commented Aug 12, 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 13, 2023

Choose a reason for hiding this comment

🔹 Simplify Code (Nice to have)

Choose a reason for hiding this comment

pullrequest bot Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pullrequest bot Aug 14, 2023

Choose a reason for hiding this comment

pullrequest bot Aug 14, 2023

Choose a reason for hiding this comment

Passing in arguments of type number does require you to offset the month by 1

Passing in a parseable date string does not require you to offset the month by 1

Choose a reason for hiding this comment

pullrequest bot Aug 13, 2023

Choose a reason for hiding this comment

🔹 Best Practices (Nice to have)

Choose a reason for hiding this comment

pullrequest bot Aug 13, 2023

Choose a reason for hiding this comment

🔹 Simplify Code (Nice to have)

Choose a reason for hiding this comment

pullrequest bot Aug 13, 2023

Choose a reason for hiding this comment

🔹 Simplify Code (Nice to have)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pullrequest bot Aug 13, 2023

Choose a reason for hiding this comment

🔸 Bug (Important)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pullrequest bot Aug 14, 2023

Choose a reason for hiding this comment

pullrequest bot Aug 13, 2023

Choose a reason for hiding this comment

🔹 Simplify Code (Nice to have)

Choose a reason for hiding this comment

pullrequest bot Aug 13, 2023

Choose a reason for hiding this comment

🔹 Style Consistency (Nice to have)

Choose a reason for hiding this comment

pullrequest bot Aug 13, 2023

Choose a reason for hiding this comment

🔸 Reduce Future Bugs (Important)

Choose a reason for hiding this comment

pullrequest bot Aug 13, 2023

Choose a reason for hiding this comment

🔸 Reduce Future Bugs (Important)

Choose a reason for hiding this comment

pullrequest bot Aug 13, 2023

Choose a reason for hiding this comment

◽ Compliment

Choose a reason for hiding this comment

pullrequest bot Aug 13, 2023

Choose a reason for hiding this comment

🔹 Style Consistency (Nice to have)

Choose a reason for hiding this comment

pullrequest bot Aug 14, 2023

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Aug 14, 2023

Choose a reason for hiding this comment

🔹 Spelling/Typo (Nice to have)

Choose a reason for hiding this comment

pullrequest bot Aug 14, 2023

Choose a reason for hiding this comment

🔹 Spelling/Typo (Nice to have)

pullrequest bot left a comment •

edited

Loading

Passing in arguments of type `number` does require you to offset the month by 1

tanmayIntelli Aug 14, 2023 •

edited

Loading

tanmayIntelli commented Aug 15, 2023 •

edited

Loading