Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor DatePicker to use React hooks #22897

Closed
wants to merge 3 commits into from
Closed

Refactor DatePicker to use React hooks #22897

wants to merge 3 commits into from

Conversation

truchot
Copy link
Contributor

@truchot truchot commented Jun 4, 2020

Description

Related to #22890

How has this been tested?

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

Types of changes

Refactor to use React hooks.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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.

@youknowriad youknowriad added [Type] Code Quality Issues or PRs that relate to code quality Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels Jun 4, 2020
@ZebulanStanphill ZebulanStanphill added the [Package] Components /packages/components label Jun 4, 2020
ZebulanStanphill
ZebulanStanphill approved these changes Jun 4, 2020
View reviewed changes
Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (looks good to merge)!

@ZebulanStanphill ZebulanStanphill self-requested a review June 4, 2020 22:16
@ZebulanStanphill
Copy link
Member

It looks like the unit test failures are legitimate. The tests need to be updated due to the changes made to the component. The tests are trying to call getMomentDate, which was previously a method of the DatePicker class. I'm not sure what the correct solution is at the moment.

@talldan
Copy link
Contributor

talldan commented Jun 5, 2020

Probably the easiest option is to extract getMomentDate so that it's defined separately to the component. The function would need to be changed to accept currentDate as a parameter.

It could then be exported as a named export from the file and imported into the test.

The other option is to keep it internal and try to make similar assertions on the output of the component itself, but that seems a bit more complicated.

@truchot
Copy link
Contributor Author

truchot commented Jun 5, 2020 •
edited
Loading

I'm not sure to understand what I have to do…

@torounit
Copy link
Member

torounit commented Jun 7, 2020

@truchot

for testing getMomentDate

  1. Move function getMomentDate out of DatePicker
  2. export getMomentDate and import in test/date.js.
  3. update line const momentDate = wrapper.instance().getMomentDate();

Base automatically changed from master to trunk March 1, 2021 15:43
@aristath
Copy link
Member

aristath commented Mar 5, 2021

This PR will need to be rebased to fix the merge conflict and also reset the tests that got stuck 👍

@Mamaduka
Copy link
Member

Mamaduka commented Sep 7, 2021

Hello, @truchot

If you have time to rebase the PR, I think we can merge your updates.

@Mamaduka Mamaduka added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 21, 2021
amustaque97 added a commit to amustaque97/gutenberg that referenced this pull request Nov 24, 2021
@amustaque97
Refactor DatePicker component to use react hooks and change it from c…
b960568 
…lass

component to function component.

It was already done as a part of different PR WordPress#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.
@amustaque97 amustaque97 mentioned this pull request Nov 24, 2021
Merged
7 tasks
ciampo added a commit that referenced this pull request Dec 15, 2021
@amustaque97 @ciampo
Refactor DatePicker component to react hooks and function component (#…
92923c6 
…36835)

* Refactor DatePicker component to use react hooks and change it from class
component to function component.

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.

* add changelog entry

* add `onMonthPreviewed` in README.md

* Remove `getMomentDate` test case

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. If we take help of `currentDate` props then test cases
are already in the place.

* Fix `onChangeMoment` test cases with help of `onDateChange` props

* Move `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

* Remove extra spaces from the changelog file

Co-authored-by: Marco Ciampini <[email protected]>

* Descriptive documentation on `onMonthPreviewed ` props

Co-authored-by: Marco Ciampini <[email protected]>

Co-authored-by: Marco Ciampini <[email protected]>
@amustaque97
Copy link
Member

Closing this PR in favour of #36835

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@youknowriad youknowriad Awaiting requested review from youknowriad

@ZebulanStanphill ZebulanStanphill Awaiting requested review from ZebulanStanphill

Assignees
No one assigned
Labels
Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Package] Components /packages/components [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@truchot @ZebulanStanphill @talldan @torounit @aristath @Mamaduka @amustaque97 @youknowriad