unmade / am-date-picker

angular material date picker
MIT License
21 stars 13 forks source link

Core Restructure #8

Closed SirDarquan closed 8 years ago

SirDarquan commented 8 years ago

So I made quite a big change with this commit so if you're unwilling to merge it, I totally understand. I'm still kind of new to git and the methodologies that should be used when performing commits, so I have all of the changes in 1 gigantic commit instead of spread out over several. This was mostly because I worked straight through until I was finished. I'm rambling... Here's the Breakdown of what was done:

Since the dialog code was detached from the directive, the watches on the min and max dates became unnecessary. They still work as expected though because both are passed to the dialog. This also meant that the test checking to see if the current date is now unselecteable didn't work they way it used to. I removed them for now but I'm thinking of how that should work going forward for the directive.

unmade commented 8 years ago

Thank for your contribution! I don't mind about separation and I use it in more complex projects. Actually, I try to follow johnpapa styleguide. Unit tests all passed fine, so I run server and open index.html in demo folder and I've encountered several problems:

  1. Month navigation doesn't work. Sometimes I can't switch to previous months, sometimes to the next one, and sometime neither to previous nor to next month. I believe condition (dialog.monthYear.isBefore(dialog.minDate, "month")) works incorrect. I understand why it is here, but I don't think it would be problem if user could list through the maxDate or the maxYear bounds. More important that he couldn't select date after the maxDate or before the minDate. In my initial concept, maxYear doesn't restrict anything. It is just the maximum available year in the year selection, same apply to the minYear.
  2. If no date selected and I press 'OK' button, the today date is selected automatically. I don't like this behavior, because this way you could select date, that before the minDate or after the maxDate. Also, there is 'Today' button for this.
  3. As suggestion, instead of Date: "=ngModel" use date: "=ngModel". JS already have Date object. I know that you didn't overwrite or hide actual Date object, but for example Atom highlights amDatePicker.Date as if it is Date object.
  4. Localization doesn't work. I think the reason is that by this dialog.moment.locale('ru'); you've set the locale locally. In current implementation locale should be set globally, or you need some additional refactor to be able to set it locally.
SirDarquan commented 8 years ago

Ok So I've fixed the first problem. I found it yesterday and all I can say is "I'm sorry". I'm going to get that in as soon as I make tests for it. AS for number two, I noticed that also and I'm working to correct that. I was hoping not to create a "selectedDate" variable but it seems that's what I'm going to have to do. The third one is easily done. And finally, localization. I feel that setting locale globally on the moment object is a 'cheat'. This component will never be used in isolation and making assumptions on how the developer using this component isn't wise. Maybe they want to show the same date/time in several different locales at the same time. Setting moment globally would prevent tat scenario. So I'll figure out how to make the local locales work.

unmade commented 8 years ago

I agree that global locale on moment object looks like bad design. I think I know how to make local locale, so if you don't have time or anything I can do it.

SirDarquan commented 8 years ago

No, I'll take care of it. Should I close this PR and open a new one with the fixes?

unmade commented 8 years ago

You could make changes and update this PR, if you'd like.

SirDarquan commented 8 years ago

Ok I fixed all the issues as promised. give it a look over and we'll go from there.

unmade commented 8 years ago

I think everything is all right. Forgot to ask last time, why do you inject $mdDialog and $q in amDatePickerService?.

SirDarquan commented 8 years ago

Oh $q was a mistake. I just noticed that one recently myself. But the reason for $mdDialog was to allow independent showing of the picker dialog eventually. I personally don't like how opinionated the amDatePicker directive is, so making a .show() method in the service will allow me and others to make our own directives but lean on the work done with the picker.

unmade commented 8 years ago

So, after learning you PR more, I find out that locals in the $mdDialog.show misses popupDateFormat: amDatePicker.popupDateFormat attr, but that not the case.

I noticed, if you click outside the date won't be updated. Of course, I understand why. But in my opinion date picker itself is not usual dialog. Most of the time user wants to pick a date and close picker as fast as possible (and most of the time user picks exactly the date that he wants to pick). So when user clicks outside the date should be updated. I mean a lot of pickers work this way, some of them close immediately (but I don't like that). There is no hard rule, but I believe, in some cases 'click outside' could stay for 'do nothing' and in some other cases for 'save current state'.

You've done a great work and I really appreciate your efforts, but I'm afraid I can't accept this PR, as we have different view on how the things should be done.

By the way, on your place I'd move the condition !dialog.maxDate || dialog.monthYear.isBefore(dialog.maxDate, 'month') from the nextMonth function to the <md-button></md-button> like this:

<md-button ng-click="dialog.nextMonth()"
                   ng-disabled="dialog.maxDate && !dialog.monthYear.isBefore(dialog.maxDate, 'month')"
                   class="md-icon-button">
    ...
</md-button>

The same applies to the previousMonth function. Otherwise it seems strange when you're clicking 'next month' button and nothing happens.

SirDarquan commented 8 years ago

Ok I respect that. I guess I'll have to go find a different date picked for my project. Thanks for letting me help

unmade commented 8 years ago

If you don't mind, please let me know which one you will choose.