Excluded dates message #4437

dvelazquez1282 · 2024-01-03T13:05:17Z

I have added the possibility of add a reason message in each exluded day

pullrequest

✅ This pull request was sent to the PullRequest network.

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

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 108
- 10

42% JavaScript (tests)
41% JavaScript
18% SCSS

Type of change

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

docs-site/yarn.lock

codecov · 2024-01-03T13:09:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3cd50f6) 95.31% compared to head (a15e551) 95.46%.
Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4437      +/-   ##
==========================================
+ Coverage   95.31%   95.46%   +0.15%     
==========================================
  Files          29       29              
  Lines        2496     2513      +17     
  Branches     1023     1026       +3     
==========================================
+ Hits         2379     2399      +20     
+ Misses        117      114       -3

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

yarn.lock

pullrequest

Did not see tests, but also suggested the code might be able to be made simpler.

My past experience with date/time code is that it is very difficult to write correctly. My preference is usually to try to make it so there is no conditional branching so that I can reduce my testing load.

John G

Reviewed with ❤️ by PullRequest

src/date_utils.js

pullrequest · 2024-01-03T13:48:23Z

src/day.jsx

@@ -326,13 +327,23 @@ export default class Day extends React.Component {

  // A function to return the holiday's name as title's content
  getTitle = () => {
-    const { day, holidays = new Map() } = this.props;
+    const { day, holidays = new Map(), excludeDates } = this.props;
    const compareDt = formatDate(day, "MM.dd.yyyy");
    if (holidays.has(compareDt)) {
      return holidays.get(compareDt).holidayNames.length > 0


Are there holidays with no names?

◽ Clarify Intent

John G

I'm sorry. I don't understand the question. I didn't any change about the holidays

pullrequest · 2024-01-03T13:48:23Z

src/day.jsx

    const compareDt = formatDate(day, "MM.dd.yyyy");
    if (holidays.has(compareDt)) {
      return holidays.get(compareDt).holidayNames.length > 0
        ? holidays.get(compareDt).holidayNames.join(", ")
        : "";
    }
+    if (this.isExcluded()) {


I think this code could be made a lot simpler if you just appended all the titles to the output if they can be fetched and have a single flow.

Eg:

titles = [] titles.append(holidays.get(compareDt).holidayNames) titles.append(excludeDates?.filter(filterFnHere)) return titles.join(", ")

◽ Simplify Code

John G

done. Thanks for your suggestion

pullrequest

Great start here. I don't have any further feedback beyond that left by John already.

Graham C

Reviewed with ❤️ by PullRequest

pullrequest

✖️ This code review was cancelled. See Details

pullrequest

✅ This pull request was sent to the PullRequest network.

@dvelazquez1282 you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.

pullrequest

I agree with the existing review comments but did not find any other issues. I do want to emphasize one of the existing comments though. I believe that this new feature should include tests before it moves forward. Other than that nice work!

James D

Reviewed with ❤️ by PullRequest

pullrequest

This cod for date exclusion looks good—I don't have anything to add beyond what the other reviewer already mentioned.

Eric E

Reviewed with ❤️ by PullRequest

pullrequest

✖️ This code review was cancelled. See Details

pullrequest

✅ This pull request was sent to the PullRequest network.

@dvelazquez1282 you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.

pullrequest

This update looks good overall — the test is helpful. I would also make sure timezone differences aren't an issue when comparing dates here.

Otherwise, I see one patch that could be simplified (noted inline) but no blocking concerns 👍

jmromer

Reviewed with ❤️ by PullRequest

pullrequest · 2024-01-08T12:49:47Z

src/day.jsx

    }
-    return "";
+    if (this.isExcluded()) {
+      if (excludeDates) {


This conditional may be redundant since optional chaining is used on L340 (excludeDates?.filter).

🔹 Simplify Code (Nice to have)

jmromer

true. I just deleted

dvelazquez1282 added 2 commits January 3, 2024 14:03

feat(excluded dates): added the possibility of add reason message

2c82e39

index

e69a378

pullrequest bot reviewed Jan 3, 2024

View reviewed changes

martijnrusschen reviewed Jan 3, 2024

View reviewed changes

docs-site/yarn.lock Outdated Show resolved Hide resolved

martijnrusschen reviewed Jan 3, 2024

View reviewed changes

yarn.lock Outdated Show resolved Hide resolved

yarn.lock files revert

2ded65a

pullrequest bot reviewed Jan 3, 2024

View reviewed changes

pullrequest bot reviewed Jan 4, 2024

View reviewed changes

pullrequest bot reviewed Jan 5, 2024

View reviewed changes

dvelazquez1282 added 2 commits January 8, 2024 09:38

code simplification to obtain title

df46a54

added test for exclude dates

56f9286

pullrequest bot reviewed Jan 8, 2024

View reviewed changes

removed conditional not necessary

a15e551

martijnrusschen merged commit 4751cd8 into Hacker0x01:main Jan 8, 2024
6 checks passed

seckie mentioned this pull request Feb 11, 2024

[@types/react-datepicker] Update types for v6.0 DefinitelyTyped/DefinitelyTyped#68437

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Excluded dates message #4437

Excluded dates message #4437

dvelazquez1282 commented Jan 3, 2024

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

codecov bot commented Jan 3, 2024 •

edited

Loading

pullrequest bot left a comment

pullrequest bot Jan 3, 2024

dvelazquez1282 Jan 8, 2024

pullrequest bot Jan 3, 2024

dvelazquez1282 Jan 8, 2024

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot Jan 8, 2024

dvelazquez1282 Jan 8, 2024

Excluded dates message #4437

Excluded dates message #4437

Conversation

dvelazquez1282 commented Jan 3, 2024

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

codecov bot commented Jan 3, 2024 • edited Loading

Codecov Report

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Jan 3, 2024

Choose a reason for hiding this comment

◽ Clarify Intent

dvelazquez1282 Jan 8, 2024

Choose a reason for hiding this comment

pullrequest bot Jan 3, 2024

Choose a reason for hiding this comment

◽ Simplify Code

dvelazquez1282 Jan 8, 2024

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 left a comment

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 left a comment

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 Jan 8, 2024

Choose a reason for hiding this comment

🔹 Simplify Code (Nice to have)

dvelazquez1282 Jan 8, 2024

Choose a reason for hiding this comment

pullrequest bot left a comment •

edited

Loading

codecov bot commented Jan 3, 2024 •

edited

Loading