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.

Sign up for GitHub

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

Jump to bottom

currency pricerange formatter #943

Merged
merged 5 commits into from
Sep 9, 2021
Merged

currency pricerange formatter #943

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
62a3eb6
currency pricerange formatter
Sep 8, 2021
e9178b9
babel transform library syntax to es5
Sep 8, 2021
20a7d4a
switch to other currency libraries
Sep 9, 2021
4598d52
feedback
Sep 9, 2021
1f6c74c
feedback
Sep 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"babel-jest": "^25.5.1",
"comment-json": "^4.1.1",
"cross-env": "^7.0.2",
"currency-symbol-map": "^5.0.1",
"express": "^4.17.1",
"file-system": "^2.2.2",
"full-icu": "^1.3.1",
Expand All @@ -37,6 +38,7 @@
"jest": "^25.5.2",
"libphonenumber-js": "^1.9.6",
"loader-utils": "^2.0.0",
"locale-currency": "0.0.2",
"lodash.clonedeep": "^4.5.0",
"postcss": "^8.2.1",
"puppeteer": "^10.2.0",
Expand Down
25 changes: 24 additions & 1 deletion static/js/formatters-internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import HoursStringsLocalizer from './hours/stringslocalizer.js';
import HoursTableBuilder from './hours/table/builder.js';
import { DayNames } from './hours/constants.js';
import { generateCTAFieldTypeLink } from './formatters/generate-cta-field-type-link';

import LocaleCurrency from 'locale-currency'
import getSymbolFromCurrency from 'currency-symbol-map'

export function address(profile) {
if (!profile.address) {
Expand Down Expand Up @@ -522,6 +523,28 @@ export function price(fieldValue = {}, locale) {
return price.toLocaleString(localeForFormatting, { style: 'currency', currency: currencyCode });
}

/**
* Returns a localized price range string for the given price range ($-$$$$) and country code (ISO format)
* @param {Object} pricerange The price range from LiveAPI entity
* @param {string} countrycode The country code from LiveAPI entity (e.g. profile.address.countryCode)
* @return {string} The price range with correct currency symbol formatting according to country code
*/
export function priceRange(priceRange, countryCode) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor nit, but I find it confusing to have a function called priceRange that takes in a priceRange. How about naming this something like localizePriceRange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think keeping the function name as priceRange would be more consistent with the other formatter functions like Formatter.price(). I can rename priceRange param to defaultPriceRange or entityPriceRange?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, defaultPriceRange or priceRangeFieldValue sgtm!

if (!priceRange || !countryCode) {
console.warn(`No price range or country code given.`);
return '';
}
const currencyCode = LocaleCurrency.getCurrency(countryCode);
if (currencyCode) {
const currencySymbol = getSymbolFromCurrency(currencyCode);
if (currencySymbol) {
return Array(priceRange.length+1).join(currencySymbol);
Copy link
Collaborator

Choose a reason for hiding this comment

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

For my own edification, what does the priceRange object look like?

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 given priceRange from entity, if it's defined, is like this: '$', '$$', '$$$', or '$$$$'.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So it's a string? The param type listed in the method signature is object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, I missed that. Updated!

}
}
console.warn(`Unable to determine currency symbol from ISO country code ${countryCode}.`);
return '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, would it be better to simply return the original price range, non-localized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it might be a bit misleading to have default dollar signs on international locations if we can't localized it so I left it blank. But I can return the original price range if you think that's fine!

Copy link
Collaborator

Choose a reason for hiding this comment

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

My first inclination is that we return the original price range. That way, there's at least something displayed. The formatter is then strictly better than just plopping the original price range on the page. Let's go with that approach for now and see if we get QA feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. Updated!

}

/**
* Highlights snippets of the provided fieldValue according to the matched substrings.
* Each match will be wrapped in <mark> tags.
Expand Down
2 changes: 2 additions & 0 deletions static/js/formatters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
hoursList,
generateCTAFieldTypeLink,
price,
priceRange,
highlightField,
getYoutubeUrl
} from './formatters-internal.js';
Expand Down Expand Up @@ -61,6 +62,7 @@ let Formatters = {
hoursList,
generateCTAFieldTypeLink,
price,
priceRange,
highlightField,
getYoutubeUrl
};
Expand Down
215 changes: 209 additions & 6 deletions static/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions static/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"babel-loader": "^8.1.0",
"comment-json": "^4.1.1",
"css-loader": "^3.4.2",
"currency-symbol-map": "^5.0.1",
"esbuild-loader": "^2.13.1",
"file-system": "^2.2.2",
"fs-extra": "^9.0.1",
Expand All @@ -32,6 +33,7 @@
"html-webpack-plugin": "^5.3.1",
"jambo": "^1.12.0",
"jsdom": "^16.4.0",
"locale-currency": "0.0.2",
"mini-css-extract-plugin": "^1.6.0",
"postcss": "^8.3.1",
"resolve-url-loader": "^3.1.1",
Expand Down
2 changes: 1 addition & 1 deletion static/webpack/webpack.prod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ module.exports = (jamboConfig) => {
{
test: /\.js$/,
exclude: [
/node_modules\//
/node_modules\/(?!(currency-symbol-map)\/).*/
],
loader: 'babel-loader',
options: {
Expand Down
Loading