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

[BUG] Deleting the value does not work as expected in NATURAL inputMode #85

Closed
dvenkatsagar opened this issue Apr 15, 2020 · 18 comments
Closed

Comments

@dvenkatsagar
Copy link
Contributor

@nbfontana, @jlabanca

First of all the feature #80 is great, so thank you for that.

There are scenarios where this feature is failing. Here are the examples.

  1. Lets say we have a value as "1.20" in the text box with the inputMode as NATURAL.
    Now when the cursor is right beside the 1 (before the decimal) like 1|.20 and when I press delete, the value becomes "120.00".

  2. Lets take the value as "1.21" in the text box with the inputMode as NATURAL.
    Now when the cursor is on the left most side of the value like |1.21 and then press and hold Delete.
    The value becomes "0.10"

Please find the sample code with v2.3.1 here: https://stackblitz.com/edit/ngx-currency-delete-natural

Please do take a look and see if it can be easily fixed.
Thank you

@jlabanca
Copy link
Contributor

jlabanca commented Apr 15, 2020

Thanks for trying it!

For #1, I think that is the correct behavior. The Delete key should delete the next character. IF the next character is the decimal, then you'd expect 1.20 to become 120, aka 120.00. I tried it in Google sheets, and I get the same results. That being said, I agree it might not be expected. What would you expect to happen in that case?

Number 2 seems like a bug. I'd expect 1.21 to become 0.21. I can look into it!

@dvenkatsagar
Copy link
Contributor Author

dvenkatsagar commented Apr 15, 2020

@jlabanca, thanks for the reply.

For No.1, I would say that it should be deleted in a more natural way. It should feel like Im editing text. So the result that I would expect is that it would become "1.00" (as the precision would kick in and set the fraction to .00). Right now it feels as if its yeeting the 20 to integer side and it does feel odd.

A similar case is let say the value is again "1.21" and we have the cursor like this 1.|21 and we press "backspace" on the keyboard. Then the result that I would expect is that it would become "0.21". Currently that also yeets the 21 to left and the value becomes "121.00" which does seem odd to me.

This is me basing my experience with Excel.

For No.2, a simpler example is, let the value be 0.21 with the cursor like this 0.|21, and we just press delete here. The value becomes "0.10".

Hope this helps.

@jlabanca
Copy link
Contributor

That makes sense. So basically, delete and backspace should jump over the decimal and delete the digit on the other side. I agree that would feel more natural. I can work on it.

@dvenkatsagar
Copy link
Contributor Author

Thank you @jlabanca

@nbfontana
Copy link
Owner

@jlabanca @dvenkatsagar Great work guys. I'm kinda busy, but a PR is very welcome.

I'll stay tuned fot any PR and I'll try to test and release any work as soon as possible.

@nbfontana
Copy link
Owner

Oh, and I really agree with both cases that @dvenkatsagar mentioned.

@dvenkatsagar
Copy link
Contributor Author

dvenkatsagar commented Apr 16, 2020

@jlabanca

There is actually one final scenario I would like to over with you.

let the value be "1.11" and the cursor is like this 1.11|

Here if we press "backspace", the cursor stays at its position and makes the value as "1.10". What I feel is that, if the cursor can be moved left when it is in the decimal Portion till it reaches the precision length, then it would feel a little more natural.

So if the user keeps pressing "backspace", the cursor will go till the decimal replacing the numbers to 0 one by one. When it reaches the decimal, if the user keeps pressing backspace, it will skip over the decimal to other side (like you mentioned above) and work the same way as it is working now(backspace when cursor being before the decimal).

First let me know what you think about this scenario.
If you think it feels right, let me know if you want take up this scenario too, else I can do it, once you've completed your changes at your convenience.

@jlabanca
Copy link
Contributor

Here is a branch that should fix all the issues you mentioned:
https://github.com/nbfontana/ngx-currency/compare/master...jlabanca:jabanca/natural_delete?expand=1

I still need to clean it up and add tests before submitting a PR, but feel free to try it out and let me know if you find any other issues with it.

The branch also fixes some other issues:

  • Currently, if you select a subset of the numbers and press delete or backspace, nothing happens. Now it will be deleted.
  • Currently, if you select most numbers plus the suffix and press delete, the entire number is cleared. This is because the code is comparing selection length to total length without considering suffix length. This is also fixed in the branch.
  • Currently, pressing backspace before a decimal or comma does nothing. Now it will delete the pre-ceeding number.
  • Currently, pressing backspace in the prefix either jumps to the end or to the second digit (depending on the version). Now it will jump to just after the prefix.
  • Currently if you press delete before a comma or a decimal, the cursor jumps over it without deleting anything. Now, the cursor jumps over it AND deletes the next digit.

@dvenkatsagar
Copy link
Contributor Author

dvenkatsagar commented Apr 16, 2020

@jlabanca

First I would like say that I really appreciate the effort that is put for this fix. Thank you very much.
Ill check this out and test it. If I find anything, Ill post a comment.

@dvenkatsagar
Copy link
Contributor Author

dvenkatsagar commented Apr 16, 2020

@jlabanca

Ok I tested this out a little, and found out that it works as expected. There is one scenario where the cursor moves unexpectedly.

The Scenario:
Lets say we have a value 1,234.56 with the cursor at the left most end (like |1,234.56). Now when I press delete, it deletes the 1 but pushes the cursor to the very end at the right (like 234.56|).
This happens in both inputModes.

The issue only occurs when there is a thousands separator in between and it only happens with options below. I dont know about other way to get this.

The options that Im using are:

{
    prefix: '',
    allowNegative: false,
    nullable: true,
    inputMode: CurrencyMaskInputMode.FINANCIAL,
}

There is another scenario that I tested. Let me know what you think about this.

Here is the scenario:

Lets say we have the value "12.21" with inputMode as NATURAL and the cursor partially selecting the "2.2" like 12.21

Now if we press "backspace" or "Delete", currently the value becomes "1.01" which is right considering the precision kicking in. But what I feel is that it should either be "11.00" or "0.11" but I dont know which is correct.
With the inputMode being FINANCIAL, I can understand that the value becomes 0.11 and does seem right but I dont know if it is correct.

Let me know what you think and Thank you again.

@jlabanca
Copy link
Contributor

I'll keep iterating on it.

For the second case you suggested, I actually made it work that way intentionally. It felt we're for 12.21 to become 11.00. It was kind of jarring. I felt that being consistent with replacing decimal values with 0s worked. It's also kind of an edge case, so I'm not sure it'll make much of a difference.

@dvenkatsagar
Copy link
Contributor Author

dvenkatsagar commented Apr 16, 2020

@jlabanca
True, I think we can leave the second case for now else we can make it look consistent by filling up the decimal portion (like how FINANCIAL mode works for that scenario) and then proceed by filling the integer value with the remaining digits.

Taking the similar example, lets say the value is 1234.56 and we delete 34.5 from this. Currently it becomes "12.06". But I think we can make the value come up as 1.26 instead. Here the decimal portion gets filled out first with 2 and 6 and then the integer side fills up with 1.

Based on the config options in place and how the text is either partially selected or not, these cases would need to be handled differently in my opinion.

If you feel its right, please do take it up at your discretion, else we can leave it for now.

Thank you

@jlabanca
Copy link
Contributor

Alright, I think everything is working as expected now. Want to take another look?

  • If selection is entirely right of the decimal, then digits are replaced with zeros
  • If selection includes the decimal, then we do what you suggested and shift the "whole" numbers into the decimal.

I'm going to write a bunch of tests for all of these different cases and then submit a PR. Let me know if you find any other oddities in the meantime!

@dvenkatsagar
Copy link
Contributor Author

@jlabanca
Yup it works as expected. It looks good to me.

Thanks!!!

@vcanuel
Copy link

vcanuel commented Apr 21, 2020

Hi,
Thanks for this.

What about these cases:

1/ Value is 1.20 in the text box with the inputMode as NATURAL.
Now when the cursor is right beside the 0 like 1.20| and when I press backspace <- nothing happens.
It would be nice if it the caret could be move to the left: 1.2|0

2/ Value is 1.20 in the text box with the inputMode as NATURAL.
Now when the cursor is right beside the 2 like 1.2|0 and when I press backspace <-. Input becomes 1.0|0
It would be nice if it the caret could be move to the left: 1.|00

What do you think @dvenkatsagar @jlabanca ?

@dvenkatsagar
Copy link
Contributor Author

dvenkatsagar commented Apr 21, 2020

@vcanuel Both cases have been addressed above and is kept in the merge. Please check the merge request provided.

@jlabanca

There is actually one final scenario I would like to over with you.

let the value be "1.11" and the cursor is like this 1.11|

Here if we press "backspace", the cursor stays at its position and makes the value as "1.10". What I feel is that, if the cursor can be moved left when it is in the decimal Portion till it reaches the precision length, then it would feel a little more natural.

So if the user keeps pressing "backspace", the cursor will go till the decimal replacing the numbers to 0 one by one. When it reaches the decimal, if the user keeps pressing backspace, it will skip over the decimal to other side (like you mentioned above) and work the same way as it is working now(backspace when cursor being before the decimal).

First let me know what you think about this scenario.
If you think it feels right, let me know if you want take up this scenario too, else I can do it, once you've completed your changes at your convenience.

@nbfontana
Copy link
Owner

The PR is now merged. I'll release a new version as soon as I start the day

@dvenkatsagar
Copy link
Contributor Author

Thank you guys for this.

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

No branches or pull requests

4 participants