urish / angular-moment

Moment.JS directives for Angular.JS (timeago and more)
MIT License
2.6k stars 397 forks source link

amDateFormat Locale Update #212

Open randdusing opened 8 years ago

randdusing commented 8 years ago

I'm using the amDateFormat filter to display a date on a Settings page. I'm passing an actual moment value rather than a string. The user can also change the language from that page. Whenever the language is changed, the filter isn't updating the language. Whenever the line of code below is run, it looks like the old locale from the existing moment is being used. I can fix this by passing a YYYY-MM-DD formatted string rather than moment object, but it seems like something the amDateFormat filter should check for.

value = amMoment.preprocessDate(value, preprocess, currentFormat);
var date = moment(value);
ReinisV commented 8 years ago

@randdusing see this issue.

The problem is that angular filters only refresh whenever the values that are passed into it (moment or string) are changed. I do not think that there is a vanilla way to make changing the locale trigger filter update, but I have no real knowledge of this codebase, so I might be wrong.

The issue also has a workaround offered, where you add moment-timezone script to your app, and update the extra parameter, which is meant for selecting timezones of the filter. This change supposedly should cause an update to trigger. It does not work for me though and including an extra script just for a workaround seems too much hassle for me anyway.

A better fix (my thoughts) would be offering to select the locale as parameter of the amDateFormat filter. I think that momentjs allows for local changes of locale, not just global ones.

@urish does that seem feasable?

ReinisV commented 8 years ago

@urish although, angular-translate does implement this functionality. They have a <h1>{{ 'TITLE' | translate }}</h1> filter, which as only argument takes in a string (translation id from translation table) and $translate.use('de'); service function which changes the underlying translation table. And using this function does trigger filter changes.

urish commented 8 years ago

@ReinisV actually, we have since fixed the issue with the filters not being updated when you change the locale:

https://github.com/urish/angular-moment/blob/master/CHANGELOG.md#090---2015-01-11

See the note about filters being stateful by default

ReinisV commented 8 years ago

@urish I created a plnkr, which shows the problem. Only the date filters added after locale change get the new locale. Could you please point out to me, what I'm doing wrong?

randdusing commented 8 years ago

@ReinisV Thanks for creating the Plnkr. With your code, the locale isn't updated. If you change moment() to moment().toISOString(), it works fine.

ReinisV commented 8 years ago

@randdusing, yes, I understand that if you put bind the filter to a function on an object (in this case, the toISOString()), the filters watch has no way of knowing whether the value it is supposed to be watching has changed or not without calling the function. So the toISOString() function is called each digest.

I think that this is unnecessary overhead. What probably happens inside is that in your case the moment object returns a string to the filter, and the filter takes that string and generates a new moment() using it. And that happens every digest.

This is what amDateFormat filter should do. It should take in a moment() object, check for changes each digest and react to them accordingly, and also check for changes in locale each iteration, even if the object has not changed at all.

In my plnkr, that does not happen. So I have to ask again, @urish what exactly is my plnkr doing wrong? Why are my filters not stateful?

urish commented 8 years ago

@ReinisV indeed, with the current version of moment and angular-moment, updating the locale does not affect existing moment() instances. A workaround for the current version (1.0.0-beta.4) would be:

app.run(["angularMomentConfig",function(angularMomentConfig) {
  angularMomentConfig.preprocess = function(value) {
    return moment(value).locale(moment.locale());
  }
}]);

See my updated plunker for an example.

ReinisV commented 8 years ago

@urish, thanks! This will make my work a lot easier :+1: It just would be nice to have if the locale change affected all existing moment() objects globally out of the box.

urish commented 8 years ago

@ReinisV great! this will be fixed for the 1.0.0 release

p1tt1 commented 8 years ago

thanks!