feat: set document lang attribute#649
Conversation
| if (!isArrayEqual(proposed, this._locale)) { | ||
| this._locale = proposed; | ||
| this.trigger('localeChanged'); | ||
| next(() => this.trigger('localeChanged')); |
There was a problem hiding this comment.
The locale does not actually change until after this setter has run through. So emitting this event during the setter causes all (synchronous) listeners accessing the locale property to receive the old value.
The reason that it works in the {{t}} helper is that recompute marks the helper as dirty and it's only recomputed later in a different run loop queue.
ember-intl/addon/helpers/-format-base.js
Line 22 in 8900971
I only noticed this "bug" now, because I used the event further down in the class.
There was a problem hiding this comment.
this should be an independent bugfix PR then, right? I guess it would also be good to have some sort of test for this so that we don't regress on this again.
| }, | ||
|
|
||
| /** @private **/ | ||
| updateDocumentLanguage: on('localeChanged', function() { |
There was a problem hiding this comment.
why do you use the Evented system for this instead of calling updateDocumentLanguage() directly from the setter?
There was a problem hiding this comment.
Because it was already there and (in theory) usable. Assuming that localeChanged was implemented correctly in the first place, this would have been a "minimally invasive" addition. 😄
Doing it this way also allows us to potentially add further listeners, without having to explicitly list them all.
But on second thought, I think you are right. The Evented thing is foreseen to be deprecated at some point (AFAIK) and the order the listeners would wbe called in is not explicitly defined.
I'll refactor it.
Turbo87
left a comment
There was a problem hiding this comment.
seems good to me. we don't have any fastboot tests in here so far, right?
|
@Turbo87 correct, no fastboot tests at the moment. |
Closes #533. Based on the code provided by @Turbo87 in #533 (comment)
Closes #533. Based on the code provided by @Turbo87 in #533 (comment)
331f0d2 to
e14df1f
Compare
|
@buschtoens I made some adjustments to fix the failing test and rebased. Merge whenever you're ready. |
|
Awesome, thanks! :) |
Closes #533.
Based on the code provided by @Turbo87 in #533 (comment)