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

feat(datepicker): datepickers in popups #715

Closed
wants to merge 1 commit into from
Closed

feat(datepicker): datepickers in popups #715

wants to merge 1 commit into from

Conversation

pkozlowski-opensource
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource commented Sep 10, 2016

This is ready for the review now. There are still a number of corner cases that we should take care about but IMO we should get it to the user hands ASAP.

I will open small, focused issues for all the remaining items as soon as this one lands.

this._writeModelValue(this._model);
}

isOpen() { return !!this._cRef; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would probably be better as a getter

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isOpen() is consistent with what we've got in other directives (dropdown, typeahead, ...). Please open a separate issue if you think that we should change it across the project.

@jnizet
Copy link
Member

jnizet commented Sep 13, 2016

This looks really great. Congrats. 👍

I'm not sure if it's one of the known bugs that you plan to fix later, so I'll report it here. From the demo, open the popup, pick a date, erase the last two digits from the input field, click elsewhere in the page, then click the addon button again: an exception is thrown (Cannot read property 'year' of null at NgbInputDatepicker.manualDateChange), and it's not possible to open the popup anymore without refreshing the page.

@pkozlowski-opensource
Copy link
Member Author

This looks really great. Congrats.

thnx @jnizet

I'm not sure if it's one of the known bugs that you plan to fix later, so I'll report it here. From the demo, open the popup, pick a date, erase the last two digits from the input field, click elsewhere in the page, then click the addon button again: an exception is thrown (Cannot read property 'year' of null at NgbInputDatepicker.manualDateChange), and it's not possible to open the popup anymore without refreshing the page.

There are still many, many corner cases around invalid dates / inputs, validation and popup opening. I will fix the one you've found but there are many more tests we should add to make it really robust. In any case we are still in alpha so as long as the happy path works I want people to play with it.

But yeh, it definitively needs more polish.

@maxokorokov
Copy link
Member

Looks quite good for the initial version!
Sure the validation/conversion/focus/blur should be improved later.

One thing that needs to be fixed for sure for me is when you type something invalid like aaa in the input and try to open, it crashes the datepicker forever with TypeError: Attempted to assign to readonly property. To me the first version should eat all invalid inputs silently and probably avoid formatting 2016 to 2016--

Another is that there is no documentation on the NgbInputDatepicker in the demo currently.

It's sad that we have all datepicker inputs in three places, but I can't see what to do with it right away...

@pkozlowski-opensource
Copy link
Member Author

@jnizet @maxokorokov @wesleycho once again thnx for all your reviews! I went through all the comments and fixed pbs or commented on your comments :-)

I've also fixed bugs you've seen - I'm sure that more remain but yeh, that's just a starting point.

@wesleycho
Copy link
Member

I'm fine with this as it is for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants