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

[NT-296] Round and format amount input based on some rules #857

Merged
merged 6 commits into from Sep 27, 2019

Conversation

dusi
Copy link
Contributor

@dusi dusi commented Sep 27, 2019

πŸ“² What

What the title says

πŸ€” Why

It was noticed that rounding was a bit off on the pledge amount field so this work should fix that.

πŸ›  How

In order to provide great user experience we do have couple rules that we not only round the value but also update the input field

  • If user enters number that ends with decimal separator we remove the separator (i.e 10.0 => `10)
  • If user enters number that ends with trailing zeros we remove zeros and decimal separator
  • If user enters number that has more than 2 decimal numbers we round it down and trim it to 2

This is so that users don't have to manually delete trailing zeros to change the value itself. As soon as there's a legit decimal number we keep it (since that one was most likely intended)

βœ… Acceptance criteria

Live user input

  • If the value being entered is lower than min value the Done button on the keyboard is disabled
  • If the value being entered is lower than min value the text color is changed to red
  • If the value being entered is higher than max value the Done button on the keyboard is disabled
  • If the value being entered is higher than max value the text color is changed to red

Dismissing keyboard (either by tapping the Done button or tapping anywhere else on the screen) while entering this input results to the following output

  • Input 10 (or similar - test with other values) produces 10
  • Input 10. (or similar - test with other values) produces 10
  • Input 10.0 (or similar - test with other values) produces 10
  • Input 10.00 (or similar - test with other values) produces 10
  • Input 10.000 (or similar - test with other values) produces 10
  • Input 10.1 (or similar - test with other values) produces 10.10
  • Input 10.01 (or similar - test with other values) produces 10.01
  • Input 10.99 (or similar - test with other values) produces 10.99
  • Input 10.999 (or similar - test with other values) produces 11
  • Input 10.129 (or similar - test with other values) produces 10.13
  • Input 10.123456789 (or similar - test with other values) produces 10.12

Stepper

  • At any time for the sample input in the table above, tapping stepper's + or - button increments/decrements the value properly

@ksr-ci-bot
Copy link

ksr-ci-bot commented Sep 27, 2019

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@@ -77,13 +77,13 @@ final class PledgeAmountViewControllerTests: TestCase {
}
}

func testView_StepperIncrementButtonDisabled_WhenStepperValueSetToMaximum() {
func testView_StepperIncrementButtonDisabled_WhenStepperValueSetToMaximumStepperValue() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed couple tests for better understanding of what min/max are we referring to (since there is stepper min/max value, pledge min/max value, etc.)


self.currency = project
.map { currencySymbol(forCountry: $0.country).trimmed() }

self.stepperMinValue = minValue.mapConst(0)
self.stepperMaxValue = minValue.mapConst(Double.greatestFiniteMagnitude)
self.stepperMaxValue = minValue.mapConst(1_000_000_000)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure what to do about this...we have to set stepper.maxValue since the default value is 100 but I'm not sure to what value. Since we allow inputs bellow pledge minimum, we should also allow inputs above pledge maximum in order to change the text color to red when the input is above that value and still allow stepper / text interactions.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine... it's an arbitrarily high number.

@@ -122,19 +122,19 @@ internal final class PledgeAmountViewModelTests: TestCase {
self.doneButtonIsEnabled.assertValues([true])

self.vm.inputs.stepperValueChanged(2)
self.doneButtonIsEnabled.assertValues([true, true])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly skipRepeats for these signals...should have done that before, only noticed now.

@dusi dusi changed the title Round and format amount input based on some rules [NT-296] Round and format amount input based on some rules Sep 27, 2019
@dusi dusi requested a review from cdolm92 September 27, 2019 01:03
Copy link
Contributor

@cdolm92 cdolm92 left a comment

Choose a reason for hiding this comment

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

G2G!!πŸ›³πŸ›³πŸ›³

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Nice, thanks!


self.currency = project
.map { currencySymbol(forCountry: $0.country).trimmed() }

self.stepperMinValue = minValue.mapConst(0)
self.stepperMaxValue = minValue.mapConst(Double.greatestFiniteMagnitude)
self.stepperMaxValue = minValue.mapConst(1_000_000_000)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine... it's an arbitrarily high number.


// MARK: - Functions

private func maximumValue() -> Double {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have this as a static property on an enum so that we can ensure it's shared between the tests and VM? Something like:

enum PledgeAmountStepperConstants {
  static let min: Double = 0
  static let max: Double = 1_000_000_000
}

@dusi dusi merged commit 08995ab into master Sep 27, 2019
@dusi dusi deleted the amount-input-rounding branch September 27, 2019 19:19
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.

None yet

4 participants