-
Notifications
You must be signed in to change notification settings - Fork 119
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
Allow empty fields #71
base: master
Are you sure you want to change the base?
Conversation
Hi @pareeohnos, |
Codecov Report
@@ Coverage Diff @@
## master #71 +/- ##
==========================================
+ Coverage 97.95% 98.21% +0.25%
==========================================
Files 1 1
Lines 49 56 +7
Branches 20 25 +5
==========================================
+ Hits 48 55 +7
Partials 1 1
Continue to review full report at Codecov.
|
Apologies @kevinongko I had left the I also ran the linter locally and it threw up some errors which the travis-ci linter isn't throwing so i resolved these as well |
@kevinongko just ran into the issue and the reason why I left it failing on the linter. If I leave in the Unsure how to proceed, as I can't see any work around without removing the |
Hey @pareeohnos Thanks for submitting the PR. Seeing as
It looks as though you are doing a check to make sure that the value is specifically |
I had originally tried that approach but Vue still spits out a warning (though it's just a warning so wouldn't cause any real issues). I assume vue's type check is checking for null before the actual type check |
Ah right, that's a pity. Do you think it would it break BC to remove the |
Unsure really, I can't see why it would break anything. The only thing is it wouldn't give any warnings or errors if the |
When do you have in mind to release this fix? Thanks in advance |
I'm happy to make any change needed but can't personally get it merged released. Hopefully soon |
Need this fix for multiple projects. Any ETA? |
@egeersoz I can't do anything about getting it merged unfortunately. @kevinongko any updates, would be good to be able to use this without needing to use my fork in our system |
@pareeohnos Your branch doesn't quite work for me. When the field has an empty value, it automatically takes |
@egeersoz have you added |
@pareeohnos Yes I tried that already. It fails the typecheck when value is null because it expects a String or Number as the value. |
@egeersoz hmm fair enough. I had originally removed this but it failed the linter specs and a PR won't be accepted with this, so not sure what to do here really |
What's the status on this? I'm still in need of this functionality |
Have anybody new's on that question ? |
This resolves #53 and #57 by allowing
null
values to be passed in and not have it replaced with a0
value. Currently, theemptyValue
attribute is only able to handle actual values, so supplyingnull
will not result in an empty field and instead defaults back to 0.This changes allows
null
to be passed to theemptyValue
property. It also allows thevalue
property to be null, as this also prevents it defaulting to 0.