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

Allow empty fields #71

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

pareeohnos
Copy link
Contributor

This resolves #53 and #57 by allowing null values to be passed in and not have it replaced with a 0 value. Currently, the emptyValue attribute is only able to handle actual values, so supplying null will not result in an empty field and instead defaults back to 0.

This changes allows null to be passed to the emptyValue property. It also allows the value property to be null, as this also prevents it defaulting to 0.

@kevinongko
Copy link
Owner

Hi @pareeohnos,
Thanks for the PR 👍
Please make sure the CI checks is all green before I can merge it

@codecov
Copy link

codecov bot commented Jun 29, 2018

Codecov Report

Merging #71 into master will increase coverage by 0.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/vue-numeric.vue 98.21% <100%> (+0.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f639863...fda2992. Read the comment docs.

@pareeohnos
Copy link
Contributor Author

Apologies @kevinongko I had left the type of the prop out because it was always giving warnings/errors in the actual code, but I've just had another look and it seems to be fine with it now, so not sure what the issue was.

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

@pareeohnos
Copy link
Contributor Author

@kevinongko just ran into the issue and the reason why I left it failing on the linter. If I leave in the type: [String, Number] attribute of the value prop, then Vue raises an error when the value is null because null is not a String or a Number. If I remove the type attribute and validate it manually, then I get around this but that is in turn causing the linter to fail.

Unsure how to proceed, as I can't see any work around without removing the type attribute

@fourstacks
Copy link

Hey @pareeohnos

Thanks for submitting the PR.

Seeing as null is an Object in JS, could you just add Object to the array on line 135? e.g.

type: [String, Number, Object]

It looks as though you are doing a check to make sure that the value is specifically null a Number or a String in the validator so you shouldn't run into problems with people adding non-null objects in.

@pareeohnos
Copy link
Contributor Author

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

@fourstacks
Copy link

Ah right, that's a pity. Do you think it would it break BC to remove the required attribute for that prop and set default to null instead of an empty string?

@pareeohnos
Copy link
Contributor Author

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 value prop was not provided by the developer but that's not the biggest issue

@serudda
Copy link

serudda commented Jul 11, 2018

When do you have in mind to release this fix? Thanks in advance

@pareeohnos
Copy link
Contributor Author

I'm happy to make any change needed but can't personally get it merged released. Hopefully soon

@egeersoz
Copy link

Need this fix for multiple projects. Any ETA?

@egeersoz
Copy link

@kevinongko ?

@pareeohnos
Copy link
Contributor Author

@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

@egeersoz
Copy link

egeersoz commented Sep 5, 2018

@pareeohnos Your branch doesn't quite work for me. When the field has an empty value, it automatically takes 0 as a value. See here: https://s15.postimg.cc/xkyg20yi3/numeric.gif

@pareeohnos
Copy link
Contributor Author

@egeersoz have you added :empty-value="null" to the template? Otherwise it defaults to the normal behaviour

@egeersoz
Copy link

egeersoz commented Sep 6, 2018

@pareeohnos Yes I tried that already. It fails the typecheck when value is null because it expects a String or Number as the value.

@pareeohnos
Copy link
Contributor Author

@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

@tryforceful
Copy link

What's the status on this? I'm still in need of this functionality

@Vacarov
Copy link

Vacarov commented Aug 24, 2020

Have anybody new's on that question ?

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.

Is null as a value possible?
7 participants