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

feat(datepicker): add datepicker-mode #1516

Closed
wants to merge 6 commits into from
Closed

feat(datepicker): add datepicker-mode #1516

wants to merge 6 commits into from

Conversation

tomchentw
Copy link
Contributor

[Update with: https://github.com/tomchentw/bootstrap/commit/00db638b35f43ef73d74f5fa1131c3a4737c06c9]

Thanks for creating SUCH a great project!!

add datepickerConfig.datepickerMode and attribute datepicker-mode on datepicker directive:

  • accepted values are one of names: ('day', 'month', 'year')
  • datepicker-mode is two-way bound attribute
  • add datepicker-mode to README

@tomchentw
Copy link
Contributor Author

Will my PR be merged? Am I doing something wrong here?

@bekos
Copy link
Contributor

bekos commented Jan 11, 2014

Hi @tomchentw. Sorry for the late response, I will review asap.

@bekos
Copy link
Contributor

bekos commented Jan 11, 2014

@tomchentw Specifying initial mode in the datepicker is something we want to add. Actually the goal here is to provide a two-way binded variable, so I would prefer a name like datepicker-mode.

About your implementation now. Firstly, an option like this should be also defined / overridden by a corresponding attribute, since I think someone will have different kind of scenarios in the same application. Also, I wouldn't expose to the user the 02 as options but rather the names dayyear.
And btw check why your build is failing and make sure that it passes when you make your changes :-)

@tomchentw
Copy link
Contributor Author

Travis CI just stopped in before_script. Is there any way that I can restart it?

The suggestion changes are:

  • change mode to names instead of numbers
  • make it overridable by datepicker-mode two-way bind attribute

@tomchentw
Copy link
Contributor Author

@bekos I've finish implement it following your suggestion, it looks great now.
I've rebase to the latest master and force update this branch accordingly. Thanks.

@tomchentw
Copy link
Contributor Author

Anyone wants to merge this?

@bekos
Copy link
Contributor

bekos commented Jan 20, 2014

@tomchentw Please be patient :-) I would need to review this again.

@tomchentw
Copy link
Contributor Author

@bekos thanks a lot. You guys are really cool :)

@bekos
Copy link
Contributor

bekos commented Jan 25, 2014

@tomchentw I am sorry but the timing was bad :-(

I really appreciate your work and this would definitely have been merged but the datepicker is already under heavy refactoring (#1599) and it will make it harder for me to merge things. This two-way binded mode was the base for this change and as you can understand your change is overlapped.

Closing and I hope you keep no hard feelings :-)
I am sure you will find something else to improve in the datepicker or any other directive. In the meantime, if you can review the refactor, that would be awesome!

@bekos bekos closed this Jan 25, 2014
@tomchentw
Copy link
Contributor Author

@bekos it's bad to hear about this since it's the first time I contribute to such a big open source project :-(.

I'll take a look at #1599 and let's make it better! Thanks a lot.

@bekos
Copy link
Contributor

bekos commented Jan 26, 2014

@tomchentw I understand! In #1599 I have some todos, you can start working on them (probably ng-if) and I will wait for a PR :-)

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

Successfully merging this pull request may close these issues.

3 participants