Skip to content

Fix for undefined values#25

Merged
timmipetit merged 2 commits intoRecras:masterfrom
1itvinka:master
Jan 5, 2016
Merged

Fix for undefined values#25
timmipetit merged 2 commits intoRecras:masterfrom
1itvinka:master

Conversation

@1itvinka
Copy link
Copy Markdown
Contributor

If the value is undefined show the input placeholder and do not show error message.

@timmipetit
Copy link
Copy Markdown
Member

Thank you for contributing to our project! Can you explain why you would want different behaviour for null and undefined?

@1itvinka
Copy link
Copy Markdown
Contributor Author

For now the input have to have initial value. I would like to have such functionality: If there is no value of datepicker selected by user, the value of the model is undefined and input placeholder is shown.

@1itvinka
Copy link
Copy Markdown
Contributor Author

But we should check for null of course.

@timmipetit
Copy link
Copy Markdown
Member

Yeah, I think there is no reason to have different behaviour for null and undefined in the timepicker. Could you maybe show an example where you would need your modifications? If we would want to merge your pull request, we also need some tests.

@1itvinka
Copy link
Copy Markdown
Contributor Author

1itvinka commented Nov 4, 2015

My case is very simple: until a user himself selects the value of timepicker no value should be selected and placeholder should be displayed. In this case the value of model is undefined. It works as it should now, but there is an annoying error in console without reason. Or I don't see one. My fix just cleans this console error. In the jquery lib you can have a timepicker without initial value as you can see in the demo http://jonthornton.github.io/jquery-timepicker/.
I'll write tests if you agree to merge this fix.
Thank you for the useful directive by the way!

@timmipetit
Copy link
Copy Markdown
Member

That makes sense. Currently we use null values for that, but I think undefined should work as well. If you write a test-case for your code, we'll merge it :)

@1itvinka
Copy link
Copy Markdown
Contributor Author

Hey @timmipetit, the tests for undefined ngModel is ready. I'm sorry it took so long for me to do. I've had tons of other urgent work to complete. If anything wrong with the tests, just tell me - I'll do my best to do it the right way. Thanks!

@timmipetit timmipetit merged commit 772db18 into Recras:master Jan 5, 2016
timmipetit added a commit that referenced this pull request Jan 5, 2016
@timmipetit
Copy link
Copy Markdown
Member

Hey @1itvinka, awesome work, thanks for contributing to this project!

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.

2 participants