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 for negative values #49

Merged
merged 6 commits into from
Jun 21, 2021

Conversation

krzysztof-zych
Copy link
Contributor

This PR will resolve #34 and add support for negative values

User can specify the negative sign with negativeSign input value (default -).

@changhuixu
Copy link
Owner

@krzysztof-zych
Thank you for your contribution. I will review it this weekend.

@changhuixu
Copy link
Owner

Thanks a lot for your pull request. Negative values can be complicated.
Please see the following screenshot based on your updates.

2021-04-10-07-06-14-Ngx-Digit-Only

The first input value is invalid, which was generated by overwriting contents in selection.
The second input could be valid, but I hope that negative values would not be turned on automatically. You can refer to "decimal", which needs to be explicitly turned on.

In one line of code, you used this.min < 0 as a part of a conditional clause. I am afraid that condition could be not guaranteed. Or maybe you have other intention by using that piece of code. In that case, could you tell me more?

Please feel free to continue working on this pull request. Thanks again.

@krzysztof-zych
Copy link
Contributor Author

Hi @changhuixu
Thanks for the review.

The first input is definitely invalid and I believe I fixed it in the last commit. The problem was related to the forecastValue function so please check it also.
I have changed some other functions and add more conditions to prevent pasting and typing negative sign in places where they should not be valid.

I do not want to use another flag to explicit say that this particular input has support for negative values, because this.min variable can control it. If we set that variable greater or equal to 0, then we won't be able to put a negative sign in the control.

Thanks again and looking for your comment.

@krzysztof-zych
Copy link
Contributor Author

Hi @changhuixu
Have you found some time to review this PR?

Thanks.

@changhuixu
Copy link
Owner

Could you pull the latest master branch? I updated an e2e test in order to prevent from breaking some existing applications.

krzysztof-zych and others added 3 commits June 21, 2021 10:33
Merge pull request from changhuixu/master
# Conflicts:
#	projects/uiowa/digit-only/src/lib/digit-only.directive.ts
…ure will not break existing applications
@changhuixu changhuixu merged commit a3a2f35 into changhuixu:master Jun 21, 2021
changhuixu added a commit that referenced this pull request Jun 21, 2021
@changhuixu
Copy link
Owner

I have published a new version v3.1.0. Please give it a try.
Also, please let me know if you want a version for Angular v11 specifically.

Currently, the Travis CI is not working due to the transition their domain names or due to other reasons. Thus the demo site is not up-to-date. I am thinking of finding a new way for CI/CD.

@changhuixu
Copy link
Owner

I also changed the default branch name to "main". FYI.

@changhuixu
Copy link
Owner

CI/CD is working now. FYI. The demo site is updated.

@patrykdawid
Copy link

Also, please let me know if you want a version for Angular v11 specifically.

Hi!
I need these changes for Angular v11.
Will you be able to make changes also in v2.x?

@changhuixu
Copy link
Owner

@patrykdawid
Could you try v2.3.0? Thanks.

@patrykdawid
Copy link

  1. v2.3.0 is not visible everywhere, e.g. it is missing here and in the changelog here.
  2. I found it on NPM here, but it also behaves differently, e.g. "@2.3.0" is missing from the installation command.
  3. This version installs correctly, but doesn't know allowNegatives :(

@patrykdawid
Copy link

1. v2.3.0 is not visible everywhere, e.g. it is missing [here](https://github.com/changhuixu/ngx-digit-only/releases) and in the changelog [here](https://github.com/changhuixu/ngx-digit-only#readme).

2. I found it on NPM [here](https://www.npmjs.com/package/@uiowa/digit-only/v/2.3.0), but it also behaves differently, e.g. "@2.3.0" is missing from the installation command.

3. This version installs correctly, but doesn't know _allowNegatives_ :(

Quick autocorrect :)
The property is allowNegatives recognized.
Point 1 and 2 valid.

Thanks!

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.

Negative value
3 participants