Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Remove use of deprecated @wordpress/date getSettings() #1039

Merged
merged 6 commits into from
Dec 18, 2018

Conversation

jeffstieler
Copy link
Contributor

@jeffstieler jeffstieler commented Dec 7, 2018

Fixes #807.

See: WordPress/gutenberg#10636

Detailed test instructions:

  1. Install and activate wc-admin
  2. Open your developer console
  3. Navigate to wp-admin > WooCommerce > Dashboad
  4. Verify that no wp.date deprecation warning is present

@jeffstieler jeffstieler self-assigned this Dec 7, 2018
@jeffstieler jeffstieler requested a review from a team December 7, 2018 23:01
psealock
psealock previously approved these changes Dec 10, 2018
Copy link
Collaborator

@psealock psealock left a comment

Choose a reason for hiding this comment

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

LGMT, although it would be nice to remove the dependency altogether considering __experimentalGetSettings().l10n is our only usage.

I suppose it depends on Gutenberg's ultimate plans for __experimentalGetSettings.

@jeffstieler
Copy link
Contributor Author

@psealock - agreed. See: 979800e

Copy link
Collaborator

@justinshreve justinshreve left a comment

Choose a reason for hiding this comment

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

The code looks good for this, and is testing well in the browser.

However, it looks like two tests are failing. Looks like maybe something needs to be defined in the test setup?

TypeError: Cannot read property 'userLocale' of undefined.

Error: No locale meta information provided.

ryelle
ryelle previously requested changes Dec 12, 2018
Copy link
Member

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Can you also add a line to the date/CHANGELOG.md with a note about this fix?

@@ -59,7 +59,7 @@ class Example extends Component {
...pkgComponents,
Component,
withState,
getSettings,
getSettings: __experimentalGetSettings,
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove this, since you're not using getSettings in the component anymore.

@jeffstieler jeffstieler changed the title @wordpress/date getSettings() is deprecated, use __experimentalGetSet… Remove use of deprecated @wordpress/date getSettings() Dec 17, 2018
@jeffstieler
Copy link
Contributor Author

This is happening on master and doesn't make the tests fail:

console.error node_modules/memize/index.js:74

Jed localization error: Error: No locale meta information provided.

@jeffstieler jeffstieler dismissed psealock’s stale review December 17, 2018 19:42

Several code changes after approval.

@jeffstieler
Copy link
Contributor Author

Thanks @ryelle @justinshreve! This is ready for another review.

Copy link
Contributor

@timmyc timmyc left a comment

Choose a reason for hiding this comment

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

This is testing well for me.

@timmyc timmyc dismissed ryelle’s stale review December 18, 2018 19:02

Changelog note added

@jeffstieler jeffstieler merged commit ed36558 into master Dec 18, 2018
@jeffstieler jeffstieler deleted the fix/807-getsettings-deprecated branch December 18, 2018 23:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants