Week picker (2nd attempt) #4288

philipkocanda · 2023-10-04T13:02:20Z

📯 Hey everyone, big news

@philipkocanda and @iMartzen have helped get this Merge Request from 2021 over the finish line, so most of the credit goes to @TiesWestendorp 👏

⚙️ Implementation

() => {
  const [startDate, setStartDate] = useState(new Date("2021/02/22"));
  return (
    <DatePicker
      selected={startDate}
      onChange={(date) => setStartDate(date)}
      dateFormat="I/R"
      locale="en-GB"
      showWeekNumbers
      showWeekPicker
    />
  );
};

📖 Resources:

🤝 We used our HackerOne Community Service Day to finalize this feature, and we're thrilled to share it today.

🚀 Enjoy the new week picker, and let's make a positive impact together

pullrequest

⚠️ This pull request was not sent to the PullRequest network because the pull request is a draft.

src/week.jsx

Zarthus

Did an initial quick pass over things!

There are some linting failures too, please don't forget them too :)
We have pull request enabled for this repository, when you undraft they should be triggered for a review.

src/stylesheets/datepicker.scss

Zarthus · 2023-10-04T13:08:29Z

src/stylesheets/datepicker.scss

  margin-bottom: -8px;
 }

+.react-datepicker__week {
+  white-space: nowrap;


have you confirmed this works responsively?

martijnrusschen · 2023-10-04T13:25:46Z

@philipkocanda @iMartzen great initiative and excited to see this coming back. Once you mark the PR as ready for review it will be sent to the pullrequest network as well.

martijnrusschen · 2023-10-04T13:26:48Z

Please check the CI output. The linter is failing because of unused imports.

codecov · 2023-10-04T14:11:28Z

Codecov Report

Merging #4288 (f1a59e8) into main (6ff581d) will decrease coverage by 1.40%.
Report is 8 commits behind head on main.
The diff coverage is 67.76%.

❗ Current head f1a59e8 differs from pull request most recent head 8b5ff3c. Consider uploading reports for the commit 8b5ff3c to get more accurate results

@@            Coverage Diff             @@
##             main    #4288      +/-   ##
==========================================
- Coverage   96.67%   95.27%   -1.40%     
==========================================
  Files          27       28       +1     
  Lines        2373     2478     +105     
  Branches      952     1003      +51     
==========================================
+ Hits         2294     2361      +67     
- Misses         79      117      +38

Files	Coverage Δ
src/calendar.jsx	`98.32% <ø> (ø)`
src/month.jsx	`96.37% <ø> (ø)`
src/week.jsx	`100.00% <100.00%> (ø)`
src/day.jsx	`96.79% <83.33%> (-0.91%)`	⬇️
src/week_number.jsx	`88.67% <86.04%> (-11.33%)`	⬇️
src/index.jsx	`92.78% <59.09%> (-1.65%)`	⬇️
test/week_picker_test.js	`0.00% <0.00%> (ø)`

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

martijnrusschen · 2023-10-04T14:52:59Z

Overall the code coverage seems a bit low, and reduces the project coverage from 96% to 94%, it would be great to make sure we keep this stable and increase the coverage on this PR to the project levels.

Zarthus · 2023-10-04T15:23:28Z

BTW: This MR would benefit from having Co Author data in the commit message.

https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors

I would also include @TiesWestendorp if some of their work is included in this MR.

Co-authored-by: Ties Westendorp <[email protected]>

philipkocanda · 2023-10-05T09:32:08Z

@Zarthus Great suggestion! I've added Ties Westendorp as the co-author on the first commit.

…cker into week-picker-2023

pullrequest

✅ This pull request was sent to the PullRequest network.

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

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 762
- 53

60% JavaScript (tests)
35% JavaScript
4% SCSS
1% Markdown

Type of change

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

pullrequest

This PR looks fantastic and is really well done. I am glad to see so much work put into unit testing. I only had a few comments about minor issues but that's all.

Andy W

Reviewed with ❤️ by PullRequest

src/index.jsx

src/week_number.jsx

test/week_picker_test.js

pullrequest · 2023-10-25T21:53:06Z

test/week_picker_test.js

+      <DatePicker onChange={onChangeSpy} showWeekPicker />,
+    );
+    expect(onChangeSpy.called).to.be.false;
+    const input = ReactDOM.findDOMNode(weekPicker.input);


ISSUE: react/no-find-dom-node (Severity: Medium)
Do not use findDOMNode. It doesn’t work with function components and is deprecated in StrictMode. See https://reactjs.org/docs/react-dom.html#finddomnode

Remediation:
this probably doesn't matter because we're inside a test, not a component, right?

🤖 powered by PullRequest Automation 👋 verified by Andy W

This is indeed deprecated behaviour and should not be used anymore.

martijnrusschen · 2023-10-26T09:16:54Z

Test coverage seems pretty decent, but src/week_number.jsx coverage seems to have reduced a lot. Can you look into keeping the test coverage stable? We have a high coverage of 96% and I would love to keep it that way. Right now, the overall package coverage would reduce by 2% if we merge this PR.

pullrequest

These changes look good and add the week picker that was described. I don't see any issues, and the changes overall look reasonable including good tests and examples.

James D

Reviewed with ❤️ by PullRequest

pullrequest

i see where you responded to my suggestion about a default in a switch statement but I have further comments on how you implemented my suggestion!

Andy W

Reviewed with ❤️ by PullRequest

src/index.jsx

…factored week_number isSameDay() method and removed usage of findDOMNode in week_picker_test

…d year_picker_test

test/year_picker_test.test.js

…_test and year_picker_test" This reverts commit f99f41a.

Zarthus

I've taken a quick look, I'll approve this now since Martijn and Pull Request seems sufficient :) My only note is that in the future I'd encourage splitting up changes (such as CONTRIBUTING) into a separate MR to reduce review overhead and increase focus, but since it's such a small change it's honestly OK.

Martijn and PR have got it from here I'm sure :)

pullrequest · 2023-11-01T02:20:29Z

test/week_number_test.test.js

-        <WeekNumber weekNumber={1} onClick={onClick} />,
+    it("should handle onClick function", () => {
+      const onClickMock = jest.fn();
+      const shallowWeekNumber = shallow(


Nit: This is now shadowing the outer variable. Probably not an issue, but generally a bad practice

🔹 Style Consistency (Nice to have)

James D

pullrequest · 2023-11-01T02:20:29Z

test/week_number_test.test.js

+          event.preventDefault();
+          event.key = "Enter";
+        }
+      });


This seems suspect, with this mock defined inline, it does not prevent changes to the actual code and system under test, that seems like it weakens the ability of this test to catch errors

🔸 Best Practices (Important)

James D

pullrequest

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

Cynthiailojeme · 2023-11-20T23:37:37Z

Hi @martijnrusschen. Thanks for taking this up.

I'd like to know if the latest merge has been published to npm. I can see the the showWeekPicker prop is available in the docs but not available in the library props.

martijnrusschen · 2023-11-21T09:42:37Z

It's released now: https://github.com/Hacker0x01/react-datepicker/releases/tag/v4.23.0

philipkocanda requested review from martijnrusschen and Zarthus October 4, 2023 13:02

philipkocanda assigned philipkocanda and iMartzen Oct 4, 2023

pullrequest bot reviewed Oct 4, 2023

View reviewed changes

philipkocanda commented Oct 4, 2023

View reviewed changes

src/week.jsx Outdated Show resolved Hide resolved

Zarthus reviewed Oct 4, 2023

View reviewed changes

docs-site/src/components/Examples/index.js Outdated Show resolved Hide resolved

philipkocanda changed the title ~~Week picker 2023~~ Week picker (2nd attempt) Oct 4, 2023

philipkocanda and others added 6 commits October 5, 2023 11:29

Week picker 2023

c1a445d

Co-authored-by: Ties Westendorp <[email protected]>

fix linter

d29400a

fix es-lint error: selector-not-notation and update to highlightAll()

b37aea4

calendarStartDay is passed in all other occurrences in this PR

949d885

remove //TODO

ad446b4

undo deprecated function upgrade

12f192f

philipkocanda force-pushed the week-picker-2023 branch from 1e80817 to 12f192f Compare October 5, 2023 09:30

iMartzen and others added 2 commits October 8, 2023 12:21

update CONTRIBUTING.MD and add tests to week

35ff390

Merge branch 'main' into week-picker-2023

89511c9

martijnrusschen requested a review from Zarthus October 9, 2023 17:58

iMartzen added 2 commits October 25, 2023 15:03

fix tests for selected and selected-keyboard classes

6bac593

Merge branch 'week-picker-2023' of github.com:Hacker0x01/react-datepi…

963cc25

…cker into week-picker-2023

iMartzen marked this pull request as ready for review October 25, 2023 13:14

pullrequest bot reviewed Oct 25, 2023

View reviewed changes

remove console.log

dec6e3c

pullrequest bot reviewed Oct 25, 2023

View reviewed changes

remove Duplicate onClick key

d7edbd1

add default case to switch in index.jsx

d5abe4b

pullrequest bot reviewed Oct 27, 2023

View reviewed changes

src/index.jsx Outdated Show resolved Hide resolved

iMartzen and others added 3 commits October 28, 2023 18:08

removed console.warn, add extra test coverage to week_number_test, re…

d0b3198

…factored week_number isSameDay() method and removed usage of findDOMNode in week_picker_test

Making use of enzyme lib instead of react-dom lib in calendar_test an…

f99f41a

…d year_picker_test

Merge branch 'main' into week-picker-2023

467a38a

martijnrusschen reviewed Oct 30, 2023

View reviewed changes

test/year_picker_test.test.js Show resolved Hide resolved

Revert "Making use of enzyme lib instead of react-dom lib in calendar…

8b5ff3c

…_test and year_picker_test" This reverts commit f99f41a.

iMartzen requested review from martijnrusschen and removed request for Zarthus October 30, 2023 08:18

Zarthus approved these changes Oct 30, 2023

View reviewed changes

iMartzen requested review from Zarthus and removed request for martijnrusschen October 30, 2023 08:34

Zarthus approved these changes Oct 30, 2023

View reviewed changes

pullrequest bot reviewed Nov 1, 2023

View reviewed changes

andrescevp mentioned this pull request Nov 10, 2023

Can we have a week picker ? #2033

Closed

pullrequest bot reviewed Nov 15, 2023

View reviewed changes

martijnrusschen merged commit 095796e into main Nov 17, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Week picker (2nd attempt) #4288

Week picker (2nd attempt) #4288

philipkocanda commented Oct 4, 2023 •

edited by iMartzen

Loading

pullrequest bot left a comment

Zarthus left a comment •

edited

Loading

Zarthus Oct 4, 2023

martijnrusschen commented Oct 4, 2023

martijnrusschen commented Oct 4, 2023

codecov bot commented Oct 4, 2023 •

edited

Loading

martijnrusschen commented Oct 4, 2023

Zarthus commented Oct 4, 2023

philipkocanda commented Oct 5, 2023

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

pullrequest bot left a comment

pullrequest bot Oct 25, 2023

martijnrusschen Oct 26, 2023

martijnrusschen commented Oct 26, 2023

pullrequest bot left a comment

pullrequest bot left a comment

Zarthus left a comment

pullrequest bot Nov 1, 2023

pullrequest bot Nov 1, 2023

pullrequest bot left a comment

Cynthiailojeme commented Nov 20, 2023

martijnrusschen commented Nov 21, 2023

Week picker (2nd attempt) #4288

Week picker (2nd attempt) #4288

Conversation

philipkocanda commented Oct 4, 2023 • edited by iMartzen Loading

pullrequest bot left a comment

Choose a reason for hiding this comment

Zarthus left a comment • edited Loading

Choose a reason for hiding this comment

Zarthus Oct 4, 2023

Choose a reason for hiding this comment

martijnrusschen commented Oct 4, 2023

martijnrusschen commented Oct 4, 2023

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

martijnrusschen commented Oct 4, 2023

Zarthus commented Oct 4, 2023

philipkocanda commented Oct 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 Oct 25, 2023

Choose a reason for hiding this comment

🤖 powered by PullRequest Automation 👋 verified by Andy W

martijnrusschen Oct 26, 2023

Choose a reason for hiding this comment

martijnrusschen commented Oct 26, 2023

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

Zarthus left a comment

Choose a reason for hiding this comment

pullrequest bot Nov 1, 2023

Choose a reason for hiding this comment

🔹 Style Consistency (Nice to have)

pullrequest bot Nov 1, 2023

Choose a reason for hiding this comment

🔸 Best Practices (Important)

pullrequest bot left a comment

Choose a reason for hiding this comment

Cynthiailojeme commented Nov 20, 2023

martijnrusschen commented Nov 21, 2023

philipkocanda commented Oct 4, 2023 •

edited by iMartzen

Loading

Zarthus left a comment •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

pullrequest bot left a comment •

edited

Loading