added ability to change language of all app-localize-behavior instances #44
Conversation
fixed demo toggles to switch on language change simplified based on feedback from notwaldorf changed an event name
|
This addresses issue #32. |
app-localize-behavior.html
Outdated
| @@ -194,6 +195,10 @@ | |||
| } | |||
| }, | |||
|
|
|||
| ready() { | |||
| this.fire('app-localize-behavior-created', this); | |||
There was a problem hiding this comment.
Please add docs for these events (see the @event annotations on L103)
There was a problem hiding this comment.
Done. Feel free to improve the wording if you feel there is a better description.
There was a problem hiding this comment.
Maybe rename this to app-localize-behavior-ready? created is another callback, that has another meaning :)
|
I think this is starting to look good! Left a bunch of nits and some questions |
| properties: { | ||
| /** | ||
| * The language used for translation. | ||
| */ | ||
| language: { | ||
| type: String | ||
| type: String, |
There was a problem hiding this comment.
Ah, sorry I missed this the first time around: I don't think you need an observer. You can just add notify: true, and this will fire language-changed
There was a problem hiding this comment.
I tried making that change, but it stops working. I suspect it is because notify is only notifying the parent and this.fire is notifying all ancestors. In my case the event must go beyond the immediate parent.
|
(also needs a rebase!) |
|
when can this pull request be expected to be merged? |
|
The PR needs a rebase, and review comments haven't been addressed, so it cannot be merged in its current state |
|
Any updates on this? |
|
Ping @mvolkmann. |
|
Sorry to say the reason I haven't been able to finalize this is that I can't get the commit squashing correct. Every time I try the result is not what I expect. |
|
Ah it's ok, you can ignore the squashing then -- I'll just do it from the
|
|
That would be great. Thanks Monica! R. Mark Volkmann
|
|
@mvolkmann you still need to do a rebase and address the review comments, though :) |
|
I thought I did address all the feedback. I just can't get the rebase to work correctly. Could you give that a shot? I'm fine with any changes you want to make to my pull request. R. Mark Volkmann
|
|
Hmm I left comments that you shouldn't fire 'language-changed' events explicitly, and you should just make the property be a notify property. |
No description provided.