Refactor DatePicker component to react hooks and function component

[amustaque97]

This PR is a part of issue #22890

Description

It was already done as a part of different PR #22897. After code review
it looks like the original author is not able to find time to reply or
address review comments. I'm doing takeover from here and will take care
of PR until it gets merged.

Changes done as a part of PR

• Using react hook useRef in the component.
• Refactor from class component to function component.

How has this been tested?

1. Edit page or post
2. Click on post schedule date
3. Change date
4. Publish post/page

Screenshots

Types of changes

Code Quality - Refactor

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[ntsekouras]

Thanks for your contribution @amustaque97! 💯

Code wise this looks good to me and I agree with the comment you shared in core chat:

After refactoring class component to function component few of the test cases have started failing. I have raised a PR. After looking at the test cases I believe we should remove those test cases because we should test component behaviour, not implementation details. What do you think?

I think @ciampo could share his thoughts about the way forward for the test changes, as he have worked a ton in components and we try to be as consistent as possible.

[ciampo]

Thank you @ntsekouras for the ping!

Hey @amustaque97, thank you for picking up this piece of work! I'm currently working on some DatePicker related changes in #36878, so maybe it's better to wait until that PR is merged, and rebase this PR to include those changes.

In the meantime, I'll give this PR a better look next week!

[ciampo]

Alright — #36878 has been merged!

@amustaque97 , would you mind rebasing this PR on top of trunk in order to include the latest changes to the component?

I also 100% agree with this comment:

After looking at the test cases I believe we should remove those test cases because we should test component behaviour, not implementation details. What do you think?

Although I'm curious to know why tests are failing, since I didn't expect code changes in this PR to cause them to fail. Nevermind, I just realised that they're expecting certain functions to exist on the component class's instance!

Code changes so far look good, but I'll go through a detailed code review after the rebase if that's ok! Thank you for working on this once again!

[amustaque97]

I just realised that they're expecting certain functions to exist on the component class's instance!

Yeah right, that's why, we should focus on overall component behavior rather than its internal functions.

I have taken merge from trunk branch. Please take a look once GitHub actions is complete.

[ciampo]

Overall, code changes look good — I've only added a couple of inline comments.

Regarding the failing unit tests, we definitely have to rewrite them, if anything to make them pass again. I personally see 3 alternatives here, each one representing a better solution than the previous one (but also a larger effort):

1. Rewrite the tests with the minimal changes to make them pass (i.e. not relying on the component being a Class component)
2. Rewrite part of the tests in order to test the component behaviour (instead of implementation details)
3. Do the same suggested in option 2, but also switch from Enzyme to React Testing Library

I appreciate that you may not have a lot of time to spend on this PR and/or you'd like to get it merged ASAP, and therefore I'd be ok with any of the 3 options above.

Finally, could you also add an entry to the CHANGELOG under the unreleased "Enhancements" section ?

Thank you!

[amustaque97]

Removing getMomentDate test cases. Since in real word scenario, in UI we never deal with getMomentDate directly so in the test case we cannot write some event or simulate any behaviour that will give access to getMomentDate method. Now if we think of taking help of currentDate props then test cases are already in the place.

If you have any better idea please share. I would love to implement them.

[mirka]

Great refactor!

Removing getMomentDate test cases. [...] If you have any better idea please share. I would love to implement them.

Since those tests are more like a unit test of the pure function getMomentDate to begin with (and doesn't even seem to be intended to test any component behavior at all), I could see the argument to move getMomentDate out of the DatePicker function and export it separately as a named export to test. But the tests aren't particularly complex and the behavior seems pretty much covered by the actual component-based currentDate tests like you say. So personally I'm fine with deletion too if you're happy with it!

Are we ready for final review?

[amustaque97]

Yeah, sure we can have a final round of review. 😄

[ciampo]

I could see the argument to move getMomentDate out of the DatePicker function and export it separately as a named export to test.\

This would be my favourite approach, especially since we already have those unit tests.

I went ahead on my local branch and made these changes, which seem to work well:

• moved getMomentDate to a separate utils.js file
• moved the getMomentDate tests under a new file, eg test/utils.js file
• changed the getMomentDate unit tests to avoid rendering DatePicker and using enzyme

Click to show diff

diff --git a/packages/components/src/date-time/date.js b/packages/components/src/date-time/date.js
index 9121fe0264..fd6869ac84 100644
--- a/packages/components/src/date-time/date.js
+++ b/packages/components/src/date-time/date.js
@@ -14,6 +14,11 @@ import DayPickerSingleDateController from 'react-dates/lib/components/DayPickerS
 import { useEffect, useRef } from '@wordpress/element';
 import { isRTL, _n, sprintf } from '@wordpress/i18n';
 
+/**
+ * Internal dependencies
+ */
+import { getMomentDate } from './utils';
+
 /**
  * Module Constants
  */
@@ -129,20 +134,6 @@ function DatePicker( {
 		keepFocusInside();
 	};
 
-	/**
-	 * Create a Moment object from a date string. With no date supplied, default to a Moment
-	 * object representing now. If a null value is passed, return a null value.
-	 *
-	 * @param {?string} date Date representing the currently selected date or null to signify no selection.
-	 * @return {?moment.Moment} Moment object for selected date or null.
-	 */
-	const getMomentDate = ( date ) => {
-		if ( null === date ) {
-			return null;
-		}
-		return date ? moment( date ) : moment();
-	};
-
 	const getEventsPerDay = ( day ) => {
 		if ( ! events?.length ) {
 			return [];
diff --git a/packages/components/src/date-time/test/utils.js b/packages/components/src/date-time/test/utils.js
new file mode 100644
index 0000000000..0f7d7a78ae
--- /dev/null
+++ b/packages/components/src/date-time/test/utils.js
@@ -0,0 +1,32 @@
+/**
+ * External dependencies
+ */
+import moment from 'moment';
+
+/**
+ * Internal dependencies
+ */
+import { getMomentDate } from '../utils';
+
+describe( 'getMomentDate', () => {
+	it( 'should return a Moment object representing a given date string', () => {
+		const currentDate = '1986-10-18T23:00:00';
+		const momentDate = getMomentDate( currentDate );
+
+		expect( moment.isMoment( momentDate ) ).toBe( true );
+		expect( momentDate.isSame( moment( currentDate ) ) ).toBe( true );
+	} );
+
+	it( 'should return null when given a null argument', () => {
+		const currentDate = null;
+		const momentDate = getMomentDate( currentDate );
+
+		expect( momentDate ).toBeNull();
+	} );
+
+	it( 'should return a Moment object representing now when given an undefined argument', () => {
+		const momentDate = getMomentDate();
+
+		expect( moment.isMoment( momentDate ) ).toBe( true );
+	} );
+} );
diff --git a/packages/components/src/date-time/utils.js b/packages/components/src/date-time/utils.js
new file mode 100644
index 0000000000..857b831ebb
--- /dev/null
+++ b/packages/components/src/date-time/utils.js
@@ -0,0 +1,18 @@
+/**
+ * External dependencies
+ */
+import moment from 'moment';
+
+/**
+ * Create a Moment object from a date string. With no date supplied, default to a Moment
+ * object representing now. If a null value is passed, return a null value.
+ *
+ * @param {?string} date Date representing the currently selected date or null to signify no selection.
+ * @return {?moment.Moment} Moment object for selected date or null.
+ */
+export const getMomentDate = ( date ) => {
+	if ( null === date ) {
+		return null;
+	}
+	return date ? moment( date ) : moment();
+};

@amustaque97 , do you think you'd be able to apply the same changes to this branch?

---

The actual refactor from class to functional component looks good to me.

I also noticed that you updated the tests with the minimal set of changes necessary to make them pass (i.e the option no. 1 from my previous comment) — I take it that you don't intend to make further changes to the tests (as suggested by options 2 and 3) ? I'm asking just to confirm.

[amustaque97]

I take it that you don't intend to make further changes to the tests (as suggested by options 2 and 3) ? I'm asking just to confirm

Yes, I don't plan as of now 😅 . I'm not getting enough time to up-skill myself to learn about react-testing-library and make those changes. My suggestion is that we can create a separate issue and assign it to me. Maybe in coming new year holidays I can plan to submit a new PR migrating existing test cases to react-testing-library.

Let me know if that make sense to you 😄

[ciampo]

Thank you @amustaque97 for your work, these changes LGTM 🚀

Yes, I don't plan as of now 😅 . I'm not getting enough time to up-skill myself to learn about react-testing-library and make those changes. My suggestion is that we can create a separate issue and assign it to me. Maybe in coming new year holidays I can plan to submit a new PR migrating existing test cases to react-testing-library.

Let me know if that make sense to you 😄

That sounds like a great plan! Please do ping me and @mirka if/when you decide to submit a PR about that — we're always happy to help (also in case you are not sure about certain React Testing Library patterns!)

[ntsekouras]

Great work here @amustaque97 ! Also many thanks to the reviewers! 💯