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

Support 2-Decimal Formatting & customizable decimal separator #192

Merged
merged 30 commits into from
Feb 11, 2018

Conversation

DarkSmile92
Copy link
Contributor

@DarkSmile92 DarkSmile92 commented Jan 25, 2018

Shows 2 fractions on decimals and makes the decimal separator region aware.

Close #190, Closes #173

@request-info
Copy link

request-info bot commented Jan 25, 2018

The maintainers of this repository would appreciate it if you could provide more information.

@hql287 hql287 changed the title Closes hql287/Manta#190, Closes hql287/Manta#173 Support 2-Decimal Formatting Jan 25, 2018
@hql287 hql287 changed the title Support 2-Decimal Formatting Support 2-Decimal Formatting & customizable decimal separator Jan 25, 2018
@hql287
Copy link
Owner

hql287 commented Jan 25, 2018

@DarkSmile92: This is not finished yet, right?

@DarkSmile92
Copy link
Contributor Author

@hql287 No I am adding the two settings right now and implement the display of currency sign before / after the amount. Will commit to PR and let you know once I'm done.
the latest tomorrow, have a lot of meetings today, sorry :(

@hql287 hql287 changed the title Support 2-Decimal Formatting & customizable decimal separator [WIP] Support 2-Decimal Formatting & customizable decimal separator Jan 25, 2018
@hql287
Copy link
Owner

hql287 commented Jan 25, 2018

@DarkSmile92 No pressure, Robin. I just added a [WIP] tag to the title, feel free to remove it once you're done with the PR. Thanks!

@DarkSmile92
Copy link
Contributor Author

Thanks @hql287 !
If you have time, please review the current changes. I think I got all relevant places but maybe I missed some places.

@hql287
Copy link
Owner

hql287 commented Jan 29, 2018

@DarkSmile92 Will do. Thanks!

@hql287
Copy link
Owner

hql287 commented Jan 31, 2018

@DarkSmile92 Robin, I just tested out your PR and this is pretty cool. Everything seems to be working as expected. I really like the currency sign placement setting. Nice touch! 👍

There're still some issues though:

  • Did you forget about Customizable decimal separator #190? I don't see the setting to choose between comma (,) and dot (.) as the decimal separator
  • If the price is 499.50 and the fraction is 2, it will print 499 which is incorrect.
  • If the decimal fraction is negative (eg -2), it will return an error. I know this is silly but it would be nice to show a dialogue rather than letting the app crash.
  • The tests are broken

@DarkSmile92
Copy link
Contributor Author

@hql287 Hung you are right, I totally forgot #190 sorry 😟
I will check the other points and edit the tests (which I also forgot to customize).
I'll commit tonight

@hql287
Copy link
Owner

hql287 commented Jan 31, 2018

@DarkSmile92 Thanks, bro! 👍

@DarkSmile92
Copy link
Contributor Author

DarkSmile92 commented Feb 1, 2018

@hql287

  • Customizable decimal separator #190 Settings for decimal separator
  • Checked with price of 499.50. Shows correct fractions when using 499.50.
  • Catched exception when fraction setting is <0 (with message for user)
    I customized the settings components to have a state "canSave". This can be used for further validations on the settings later.
    It throws a warning notification and does not let the user save if decimalFractions is < 0
  • Tests customized

@@ -17,3 +17,9 @@ export const saveSettings = createAction(
ACTION_TYPES.SETTINGS_SAVE,
data => data
);

// Notify user about invalid decimal fractions
export const notifyInvalidDecimalFractions = createAction(
Copy link
Owner

Choose a reason for hiding this comment

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

Please use dialog for validation message. I would like to user to acknowledge this before moving on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed :)

@hql287
Copy link
Owner

hql287 commented Feb 1, 2018

@DarkSmile92
Copy link
Contributor Author

@hql287 Already checked, you should be able to push to my PR Hung 👍

@hql287
Copy link
Owner

hql287 commented Feb 2, 2018

@DarkSmile92 I just added some new definitions to facilitate the changes in this PR. Can you do the translation for those term on Crowdin (I would like to add German support with this PR)?

Thanks!

@DarkSmile92
Copy link
Contributor Author

@hql287 Sure, translation on Crowdin done :)

hql287 added 13 commits February 2, 2018 23:40
Currency settings now hold these 4 settings as default:
- Currency (code)
- Separator (thousand & decimal)
- Sign placement
- Decimal fraction

Refactored FormReducer, Invoice & Currency component (in Settings page) with these changes
+ All operations related to constructing a new invoice has been moved to form helper. This makes sencse since most of them are already here and testing will be easier, too.
Also changed name to formatNumber
+ Invoices Container:
+ Invoice (Invoices Page) : display currency correctly
+ Settings Reducer: removed unnecessary selectors
@hql287
Copy link
Owner

hql287 commented Feb 5, 2018

Almost done.

  • Validation rule (and tests) for currency in Form/Setting
  • Test for formatNumber helper
  • Mmigration scheme for exsiting invoices

@DarkSmile92: If you can review the changes, that would be great! 👍

@DarkSmile92
Copy link
Contributor Author

@hql287 sorry for not responding, something came up at work that needed my full attention. I reviewed the changes. I really like that you made currency an object with decimal and placement properties. This makes it extensible. All changes look good to me to I agree. Can't approve them via git review so take this as review succeeded and approved:)

@hql287
Copy link
Owner

hql287 commented Feb 9, 2018

@DarkSmile92 Thanks, bro 👍

@hql287 hql287 changed the title [WIP] Support 2-Decimal Formatting & customizable decimal separator Support 2-Decimal Formatting & customizable decimal separator Feb 10, 2018
@hql287 hql287 merged commit e64f2dd into hql287:dev Feb 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants