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.

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

Remove obsolete props being passed through to DatePicker component #11649

Merged

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented Nov 8, 2018

Description

The <DateTimePicker/> component (and documentation) describes passing through locale and is12Hour props through to the <DatePicker /> component. However, <DatePicker /> does not implement (nor pass through) either of those props. Further, locale is not used anywhere.

I'm guessing these are carryovers from before the Date picker refactor.

I think locale is no longer needed because react-dates (which <DatePicker/> has as a dependent) uses moment.locale() for its localization and wp.date initializes moment with the locale via embedded server inline script. However, while this pull doesn't address this, I think this may be something needing addressed at some point because:

  • This is a hidden functionality that is not easily discoverable. If moment.locale fails to be initialized (or moment gets dropped as the date library used by wp.date) then the date-picker will not be localized (and it won't be readily apparent).
  • react-dates has some user interface strings that don't appear to be localized at all. I'm not sure if this is a biggy because I don't know if we have it configured so any of those strings are shown. If however they are or do get used, they should probably be localized.
  • DateTime does not pass through any props to the underlying react-dates component. It may have been intentional to not expose additional props controlling it to parent components, but in case it wasn't intentional we may want to consider doing so.

How has this been tested?

  • Verified via browser testing that the existing date-picker used by post-scheduler is not affected
  • ran unit tests locally to ensure no tests were broken.

Types of changes

This has minimal impact because the props removed were not being used.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@nerrad nerrad requested a review from tofumatt November 8, 2018 23:57
@nerrad nerrad self-assigned this Nov 8, 2018
@nerrad nerrad added [Package] Components /packages/components [Type] Bug An existing feature does not function as intended labels Nov 8, 2018
@tofumatt
Copy link
Member

tofumatt commented Nov 9, 2018

If moment.locale fails to be initialized (or moment gets dropped as the date library used by wp.date) then the date-picker will not be localized (and it won't be readily apparent)

We have already removed moment from the public API of wp.date–out of curiosity, did you test this with other locales?

@nerrad
Copy link
Contributor Author

nerrad commented Nov 9, 2018

We have already removed moment from the public API of wp.date–out of curiosity, did you test this with other locales?

I realize moment has been removed from the public API of wp.date which is to my point. With it being a hidden (internal only) implementation, and the reliance of react-dates on moment.locale having been set (which we do in client-assets.php via wp.date.setSettings which in turn sets locale data on moment.locale())... this is potential for bugs down the road if wp.date stops using moment internally.

I did not test this specifically with other locales because even if its NOT working, its already broken because the locale prop simply isn't being used.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@youknowriad youknowriad added this to the 4.4 milestone Nov 12, 2018
@youknowriad youknowriad added [Type] Code Quality Issues or PRs that relate to code quality and removed [Type] Bug An existing feature does not function as intended labels Nov 12, 2018
@nerrad nerrad merged commit 0c734e1 into master Nov 12, 2018
@nerrad nerrad deleted the fix/obsolete-props-passed-through-on-DateTimePicker-component branch November 12, 2018 14:44
nerrad added a commit that referenced this pull request Nov 12, 2018
## Description
<!-- Please describe what you have changed or added -->

I merged #11649 to master but the changelog entries ended up getting merged to the wrong section.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants