-
Notifications
You must be signed in to change notification settings - Fork 79
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
fix(gtfs.yml): add more currency types for fares #551
Conversation
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.
We should add all possible currency types while we're at it. I think it'd be useful to create some kind of script that can automatically output all of these values. Perhaps we can use the currency-codes package as a data source.
Yea, perhaps you're right @evansiroky. |
Instead of keying them in manually, can that be retrieved from some other source? |
Actually, I'm not really sure including all of them is warranted @evansiroky and @binh-dam-ibigroup. Having hundreds of values really just clutters the dropdown immensely. Sure, specifying some obscure currency is valid, but if our primary users of the system are North American transit agencies, does it really help to have "Platinum" as a currency type in here? |
I agree with @landonreed, it's fine to key in a few like in this PR, but it doesn't make sense to key in everything, especially if we just copy from some other place. |
I'm still a fan of allowing all possible values. The GTFS spec is clear that any ISO 4217 currency code is a valid value. Since we're making software that produces GTFS feeds I don't think it makes sense to exclude valid possible values especially when most values can easily be hidden in a react-select combobox. Go ahead and overrule me if you like though. |
@evansiroky, how do you propose we hide the values with react-select? I'm open to the idea. Feel free to jump on and add commits to #566. |
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.
Approving for now, but #566 is on my backburner and should be implemented eventually.
Checklist
dev
before they can be merged tomaster
)Description
fixes #550 by adding top currencies traded in world (and major North American currency types).