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

Updating countries to include the 83 missing currencies. #2

Merged
merged 6 commits into from
Jul 21, 2017
Merged

Conversation

dfenske
Copy link
Contributor

@dfenske dfenske commented Jul 19, 2017

Also added functionality to just get the list of currencies. Total of 174 currencies now.

…unctionality to just get the list of currencies.
@dfenske dfenske requested review from ncua and ahaug1289 July 19, 2017 19:06
Copy link
Contributor

@ncua ncua left a comment

Choose a reason for hiding this comment

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

Be great to have Andrew go through this and make sure we still want to allow users to select from all these currencies. I'm guessing we should at least remove those that have been replaced (i.e. ESP replaced with EURO)

currency-map.js Outdated
"symbolFormat": "CZK {#}"
},
"FIM": {
"name": "",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just starting looking up some of these with a blank name and they were replaced with the EURO. Should we remove these from the list? I don't think we'd want users to ever select these right? @ahaug1289 how would we handle these from a data perspective?

Copy link
Contributor

Choose a reason for hiding this comment

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

Those 4 blank name currencies are all old - and have been replaced by the Euro. They should not be selectable from the survey.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

currency-map.js Outdated
"symbolFormat": "CZK {#}"
},
"FIM": {
"name": "",
Copy link
Contributor

Choose a reason for hiding this comment

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

The extra currencies after zimbabwe seem to check out. The current answervalues that we should no longer be asking for are:
Lithuanian Lita (LTL)
Latvian Lat (LVL)
Estonian Kroon (EEK)
Slovak Koruna (SKK)
Cyprus Pound (CYP)
Maltese Lira (MTL)
Sudanese Dinar (SDD) - Replaced by Sudanese Pound (SSP) - needs to be added
Surinam Guilder (SRG) - Replaced by Surinam Dollar (SRD) - needs to be added
Reuters (SDP)
Slovenian Tolar (SIT)

The two labeled currencies above need to have new answervalues confirmed before they would be save from the survey. I can confirm the values tomorrow.

The rest of the currencies have been replaced by the euro or another currency we already have and can just be removed from the available options.

@@ -26,7 +27,20 @@ function formatCurrency(value, currencyAbbr) {
return `${value} ${currencyAbbr}`;
}

function getCurrencyList() {
return currencyMap;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this instead return the list of currency names (and sorted) since that's how it would be displayed in a dropdown?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly? I didn't know how to order it... alphabetically? Or with usd at top? and I wanted to keep the details about each currency in what we return (symbol, abbr, etc.), so I didn't just want to return a list of strings. But if you think i should change it from a single object to an array of objects, that could be possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm not understanding how this will be used for the currency dropdown. Whatever you think is best, let's go with that 😄

@dfenske dfenske merged commit c895766 into master Jul 21, 2017
@dfenske dfenske deleted the CAR-4909 branch July 21, 2017 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants