Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(datepicker): works without ng-model #1913

Closed
wants to merge 1 commit into from

Conversation

Foxandxss
Copy link
Contributor

This was one of the things we needed to fix on datepicker for the next version. It just needed a guard to check that there is a ngModel present.

For the tests, well, that is a hard part. There is no difference visually on the datepicker without ng-model, the only difference now is that it doesn't throw an exception :P. So I decided to test that the actual month is showed.

Testing for actual date is pretty hard because you can't hardcode anything so I decided to check that the title is the correct one.

I had to improvise a little bit on the year one :P

Ping @bekos

@Foxandxss
Copy link
Contributor Author

This one got rebased @bekos :)

@bekos
Copy link
Contributor

bekos commented May 7, 2014

Continuing the discussion from #2157 I am not sure how much effort will need to maintain in the future a working datepicker without the ng-model and I don't believe anyone is using it without one. My opinion is to always require it, but I want to hear your opinions also.

@Foxandxss
Copy link
Contributor Author

@bekos that was a change you introduced after the big refactor. Earlier it was required (or it would throw an error) but it was changed to allow non usage of ng-model. I would say that now that we have the option to use it without ng-model, we could just merge this and move on. The alternative is to check again for the existence of the ng-model or throw an error.

@bekos
Copy link
Contributor

bekos commented May 7, 2014

@Foxandxss Optional ngModel for datepicker is there since 0.5 if I remember correctly. So it shouldn't throw an error before, and if it was, this was a bug that noone found, because as I said I don't think anybody is using it without ng-model. It would be nice of course to have it work without ng-model, but again there are two things I consider.

  1. "Polluting" the code which is already quite big with statements that you are not sure why exist
  2. The effort vs gain to support a feature that nobody actually is using

And if 1. is easily solved if we just noop every ngModel method that we use, instead of adding conditional statements, I am not sure about the 2. since we already struggle with our time.

@pkozlowski-opensource
Copy link
Member

@Foxandxss I think I'm with @bekos here. If there is a future that we can see it is not used (as it is broken so people can't use it, if I'm not mistaken) let's remove it. If it is needed we can easily add it back.

@Foxandxss
Copy link
Contributor Author

@pkozlowski-opensource at the end of the day @bekos is the maintainer of datepicker. He introduced the ability to have optional ng-model, I just fixed it because was broken. He can revert those changes and we are good to go :)

lnmdigital pushed a commit to lnmdigital/bootstrap that referenced this pull request May 9, 2014
@chrisirhc chrisirhc modified the milestones: 0.12, 0.13 Nov 16, 2014
@wesleycho
Copy link
Contributor

I'm not sure we should support optional ng-model on datepickers. This looks harmless enough, but I can't forsee a situation where someone would want a datepicker but have no model attached to it. Even so, we could just require ng-model be used, even if for dummy purposes.

I feel like a better change would just be to enforce ng-model's presence.

@chrisirhc chrisirhc modified the milestones: Backlog, 0.13.0 Mar 23, 2015
@chrisirhc
Copy link
Contributor

I agree with @wesleycho . We should probably require ngModel instead. I also don't think this is a priority for 0.13, so moving this to Backlog.

@karianna
Copy link
Contributor

merge conflicts as well.

@Foxandxss
Copy link
Contributor Author

This one won't be merged, i leave it open as a reminder

@chrisirhc chrisirhc modified the milestones: Purgatory, Backlog Mar 27, 2015
@wesleycho wesleycho closed this in e04b06d Aug 8, 2015
@Foxandxss Foxandxss deleted the fix-model-dp branch September 8, 2015 10:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants