Currency: support symbol-alt-narrow #631
Conversation
Adds support for alternative symbols via 'symbolForm' option Fixes #479
|
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
|
Thanks for your contribution. I will review your PR this weekend, but I've left a couple of inline notes about indentation. |
| ]); | ||
| currency | ||
| ]), | ||
| symbol = currencySymbols[options.symbolForm || "symbol"] || currencySymbols.symbol; |
There was a problem hiding this comment.
Coding style: use spaces after [ and before ] like [ options..."symbol" ].
|
Documentation needs update: https://github.com/jquery/globalize/blob/master/doc/api/currency/currency-formatter.md |
| @@ -19,7 +19,8 @@ define([ | |||
|
|
|||
| var accounting = { style: "accounting" }, | |||
| code = { style: "code" }, | |||
| name = { style: "name" }, | |||
| name = { style: "name" }, | |||
| narrow = { symbolForm: "symbol-alt-narrow" }, | |||
There was a problem hiding this comment.
Let's simplify the API option. Instead of the above, let's use { form: "narrow" } and have the formatter to include the symbol-alt- part when necessary.
There was a problem hiding this comment.
Re-thinking about it, options.form could cause confusion. With options.symbolForm we make it very explicit that this form is only available when options.symbol is "symbol".
|
There is a functional test, which is good. A bonus point would be adding a unit test too. |
|
I've left a couple of comments now, just let me know if you have any questions and thanks so far. |
|
I've made the review updates myself (at https://github.com/rxaviers/globalize/tree/pr-631-continued). Although, due to Foundation changes and its updated CLA, I have been recommended not merge this PR (link) (cc @kborchers) until this is re-signed. |
|
Thanks @gethinwebster |
|
Superseded by #738 |
Adds support for alternative symbols via 'symbolForm' option Ref globalizejs#479 Ref globalizejs#631 Ref globalizejs#738
Adds support for alternative symbols via 'symbolForm' option
Fixes #479