-
Notifications
You must be signed in to change notification settings - Fork 41
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
(WIP) Add "localizePrice" helper #58
base: master
Are you sure you want to change the base?
Conversation
helpers/localizePrice.js
Outdated
|
||
if (!common.isObject(price) || !common.isString(price.currency) || isNaN(price.value)) { | ||
// Return empty string if this does not appear to be a price object | ||
return '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we return original data without modification here instead?..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case the assumption is that the theme developer is passing us something incorrect (a completely invalid object, a product image, something else) so we typically "fail" to give the theme developer some clue that they've done something wrong - and protect ourselves against attacks and stuff. That's the pattern in other helpers, but we don't have to follow it here.
I do consider prices to be "mission critical" for rendering a page, so perhaps a behavior that always tries to return something intelligible is better.
@mattolson @junedkazi interested in your perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to failing over blind defaults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that if a user calls localizePrice
with an invalid price object, we should fail. In the handlebars helper world, that means returning empty string (for consistency with behavior of other helpers, for example https://github.com/bigcommerce/paper-handlebars/blob/master/helpers/money.js#L25).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need semicolon
b0bc634
to
ee23f63
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bookernath it's looking promising :)
const common = require('./lib/common.js'); | ||
|
||
const factory = globals => { | ||
return function(price, locale) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the locale value (coming from Stencil context) takes into consideration of the locale support by the theme? i.e.: The browser could forward an accept-language
header that the theme could not fulfil. In that case, the price would be localized according to the locale value but not the rest of the page. But I guess it's not really a big issue (as it's understandable by the shopper)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it wouldn't take that into account; it's a straight representation of our parsed result of the browser's Accept-Language
header.
That being said, I don't think that's a bad thing - localizing price display is a different task than localizing all human-readable language, and perhaps we can consider those tasks separately?
dc124f8
to
41ff0d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me. Please update with tests, and add a ticket to make sure we have NODE_ICU_DATA available to Renderer.
helpers/localizePrice.js
Outdated
|
||
if (!common.isObject(price) || !common.isString(price.currency) || isNaN(price.value)) { | ||
// Return empty string if this does not appear to be a price object | ||
return '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that if a user calls localizePrice
with an invalid price object, we should fail. In the handlebars helper world, that means returning empty string (for consistency with behavior of other helpers, for example https://github.com/bigcommerce/paper-handlebars/blob/master/helpers/money.js#L25).
helpers/localizePrice.js
Outdated
|
||
if (!common.isObject(price) || !common.isString(price.currency) || isNaN(price.value)) { | ||
// Return empty string if this does not appear to be a price object | ||
return '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need semicolon
e75584e
to
c20bd16
Compare
c20bd16
to
6121e26
Compare
89de5b8
to
86fcff1
Compare
86fcff1
to
8064686
Compare
@bookernath what are the implications here for open ticket DEVDOCS-1003, handlebars-helpers-reference | Update with localizePrice Helper (Pending Merge)? |
Hi @slsriehl , In general it means we should not document this capability until it's actually live. This PR is sort of just a prototype for now, and might not be live for a while. So it effectively remains "on hold" for now from a documentation perspective. If that's clogging up tubes on your end, feel free to close things as "won't do" for now and we can create a new documentation ticket later when it's needed. |
What? Why?
Adds a new
localizePrice
helper which can be used to automatically configure the display of prices with the magic of JavaScript's Internationalization API.This helper will override the behavior of store currency settings, and display prices differently according to the provided shopper locale - which can be supplied from browser language header we receive and make available via
request.locale
, or it can be specified statically in the theme. The helper will fall back to the normal stencil "formatted" price if we get any funky input.The idea is that this will provide "magical" localization capabilities to Stencil, at least where we render pricing.
Since we're just using vanillaJS here, we can perfectly port this method to the frontend as well - for example where we render PDP pricing from an API call. So there's an opportunity for parity there.
Depends on:
https://github.com/bigcommerce/bigcommerce/pull/30545
https://github.com/bigcommerce/bigcommerce/pull/30582
Usage example
Assume a price object that looks like this:
And a request object that looks like this:
Usage would look like this:
{{localizePrice price.without_tax request.locale}}
And output would be:
$23.00
But if the request.locale changed to
fr-FR
, or if I manually specified it as such, this would output:23,00 $US
How was it tested?
Looking for feedback on approach before I add tests.
cc @bigcommerce/storefront-team @megdesko @bigcommerce/multi-currency @davidchin