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

Framework: Add localization helpers to global wpcom instance #3348

Merged
merged 2 commits into from
Feb 18, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Feb 16, 2016

This pull request seeks to add a new withLocale helper function to the global wpcom.js instance used across Calypso. Specifically, this will enable opt-in localization of REST API requests for endpoints supported in wpcom.js. This is similar to the _sendRequestWithLocale method available to undocumented endpoints, but (a) applies to any and all requests made through the wpcom.js instance and (b) leverages currentUser Redux state in place of legacy lib/user methods.

See included README.md for more information

Example usage:

import wpcom from 'lib/wp';

wpcom.withLocale().site( siteId ).postTypesList().then( ( data ) => {
    // `data` is a localized response
} );

Implementation notes:

This follows a very similar approach to the support user wrapping implemented by @jordwest in #2510.

Testing instructions:

No existing usage of the global wpcom.js have been migrated here to opt-in to the new withLocale function. Instead, ensure that existing usages remain unaffected by verifying that there are no console errors, and network requests do not include a locale parameter (unless a custom implementation has been used).

Also ensure that included Mocha tests pass by running make test from the project root directory or directly from client/state/current-user and client/lib/wp/localization.

/cc @retrofox , @timmyc (re: #3264 (comment))

@aduth aduth added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. i18n labels Feb 16, 2016
* Given a WPCOM parameter set, modifies the query such that a non-default
* locale is added to the query parameter.
*
* @param {Object} params Original parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

add - ?. I've gotten this suggestion before from @rralian

/**
 * @param  {Object} params - Original parameters
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hyphen itself is optional, and is available to help readability (reference), though a similar effect is achieved via aligning spaces on the start of the type, name, and description.

Usage seems a bit mixed throughout Calypso. In my own case, I'm using the Sublime Text 3 DocBlockr package, which does not add the hyphens and (as far as I can tell) does not offer an option to do so, so it's a bit more effort for me to revise. TL;DR: I'm lazy 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

:-D

@retrofox
Copy link
Contributor

I've been playing with this one. It looks really good.
I did two requests, the second one calling the helper and I could see locale=esin the query string.

image

image

Also the tests were passed successfully.

I just wonder if we should move this code to wpcom.js.

@retrofox
Copy link
Contributor

btw I've added a @jsdoc suggestion. Feel free to ignore it.
LGTM 🚢 👍

@retrofox retrofox added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 17, 2016
@aduth
Copy link
Contributor Author

aduth commented Feb 18, 2016

I just wonder if we should move this code to wpcom.js.

Now that you mention it, I think there is some opportunity to move bits of this to wpcom.js, though there'd still be need for a Calypso-specific override to achieve the "implicit" locale via current user settings.

We could port the withLocale prefix, though it would be a requirement to specify a value, e.g.

wpcom.withLocale( 'es' ).me().get();
// vs.
wpcom.me().get( { locale: 'es' } );

Maybe instead it could be part of a (new) initializer options object so that the a shared instance would always make requests with a particular locale?

const wpcom = new WPCOM( { locale: 'es', token: '...' } );

wpcom.me().get();
wpcom.site( '...' ).get();

@aduth aduth force-pushed the update/wpcom-with-locale branch from 4004c18 to aacb3b9 Compare February 18, 2016 14:35
aduth added a commit that referenced this pull request Feb 18, 2016
Framework: Add localization helpers to global wpcom instance
@aduth aduth merged commit 3ceb330 into master Feb 18, 2016
@aduth aduth deleted the update/wpcom-with-locale branch February 18, 2016 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants