-
Notifications
You must be signed in to change notification settings - Fork 5
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
QA item: use site locale for price range as default #949
Conversation
- update priceRange formatter function to determined the price range string in following order: - use provided country code to determine the currency symbol - if country code is invalid or undefined, use site locale to determine the currency symbol - if all else fails, return the default price range, which is in $ TEST=auto add more cases in jest test, all passed
if (currencySymbol) { | ||
return defaultPriceRange.replace(/\$/g, currencySymbol); | ||
} | ||
} | ||
console.warn(`Unable to determine currency symbol from ISO country code ${countryCode}.`); | ||
const { region, language } = parseLocale(_getDocumentLocale()); |
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 getDocumentLocale need to be updated to use parseLocale?
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.
I think it's fine to keep it as is since there are multiple locations that invoke this method to get the locale as a single string as is. If there's a place, like in priceRange, that requires some parsing then they can use parseLocale to get each sections.
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.
ok sounds like the other formatters are ok with chinese!
TEST=auto
add more cases in jest test, all passed