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

fix issue with en-IE currency #569

Closed
wants to merge 2 commits into from
Closed

fix issue with en-IE currency #569

wants to merge 2 commits into from

Conversation

WesGabbard
Copy link
Contributor

Ran into issue when trying to format currency for Ireland. When setting culture as en-IE was getting the following error:

Currency position should be among ["prefix", "infix", "postfix"]

Looking at the en-IE.js file in the languages folder, it is missing a property defining the position of the currency symbol. Added this to be prefix since Euro symbols go in front. Updated the test to account for this.

@WesGabbard
Copy link
Contributor Author

@BenjaminVanRyseghem, just checking in on this PR and see if there is anything else needed on my end. Thanks.

@WesGabbard
Copy link
Contributor Author

@BenjaminVanRyseghem checking in on this PR, is there any reason this can't be merged? Would like to be able to format currency for Ireland and in its current state I am not able using this library. This is fairly small update that should resolve the issue. Thanks.

@BenjaminVanRyseghem
Copy link
Owner

Sorry, I am very busy right now, I actually have an epic deadline next thursday...

I'll have a look as soon as I have 5' 😄

Sorry for the inconvenience

@WesGabbard
Copy link
Contributor Author

@BenjaminVanRyseghem, can this PR be merged? Thanks

CHANGELOG.md Outdated
@@ -4,6 +4,7 @@

### 2.3.1

- Fix issue with en-IE currency by adding currency position of `prefix`

Choose a reason for hiding this comment

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

wrong format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that, pushed change to add PR #

@@ -62,3 +62,4 @@ Thomas Obermüller (https://github.com/thomas88)
Tim Fish (http://github.com/timfish)
Tim McIntosh
Tim Schauder (https://github.com/cornflakeboy)
Wes Gabbard (https://github.com/Blackbaud-WesGabbard)

Choose a reason for hiding this comment

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

Move it to another commit please, so that possibly reverting your code won't remove you from the authors list 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@BenjaminVanRyseghem
Copy link
Owner

do you have any documentation showing that the currency is prefix? (to avoid another PR tomorrow to change it as it happened already 😄)

Sorry for the delay, and thanks for the PR

@WesGabbard
Copy link
Contributor Author

CHANGELOG.md Outdated
@@ -4,6 +4,7 @@

### 2.3.1

- Fix #569: Fix issue with en-IE currency

Choose a reason for hiding this comment

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

Still missing the Thanks to ... at the end 😄

@BenjaminVanRyseghem
Copy link
Owner

Thanks for the correction.

Could you please rebase your commits so that there are only 2:

  • one with the code and the changelog
  • one with the AUTHORS

thanks!

@BenjaminVanRyseghem
Copy link
Owner

BenjaminVanRyseghem commented Feb 3, 2021

One test failed:

 en-IE formats currency correctly

    Expected '-€1000.23' to be '€-1000.23', 'Should format currency correctly -1000.234 with $0.00'.

edit: the test is broken obviously, but that's for another PR 😄

@WesGabbard
Copy link
Contributor Author

My bad, long day. I should have at least made sure the unit tests passed locally before pushing up.

@BenjaminVanRyseghem
Copy link
Owner

you removed package-lock.json. Could you put it back? 😄

@stephenmcbride
Copy link

@Blackbaud-WesGabbard any chance you could re-visit this PR to make the changes requested? As an Irish dev I'm keen to see it merged 😄

@WesGabbard
Copy link
Contributor Author

@stephenmcbride @BenjaminVanRyseghem closing out and moving to #597

@WesGabbard WesGabbard closed this Apr 29, 2021
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.

None yet

3 participants