Re-render withLocalization-Components when l10n changes #196
Conversation
|
Yay, thanks! I'll review this tomorrow. |
|
Great thanks :) Looks like the linter doesn't like my code, but it talks about a line that doesn't exist. Plus locally |
| return createElement( | ||
| Inner, | ||
| Object.assign({ getString: this.getString }, this.props) | ||
| Object.assign({ getString: (...args) => this.getString(...args) }, this.props) |
There was a problem hiding this comment.
I think the linter complains about this line. Note that this change shouldn't be needed because this.getString is bound to this in line 9. That said, I'd much rather get rid of that bind and the whole constructor and fix the line length here :)
There was a problem hiding this comment.
Oh, that line does exist, I was on the wrong branch, sorry 🙈 I pushed a new commit, which also added a comment and removes the constructor.
and drop constructor which isn't needed anymore
| Inner, | ||
| Object.assign({ getString: (...args) => this.getString(...args) }, this.props) | ||
| Object.assign( | ||
| // getString needs to be re-bound on updates to trigger a re-render of Inner |
There was a problem hiding this comment.
Fixed and broke the build in the same commit 😁
|
Sweet, thanks! Can I squash this while merging? |
|
Sure! Always feel free to squash my commits 😁 |
|
I'm close to fixing #192, so I'll give myself another day. Thanks for the patience! |
|
I published |
|
Awesome, thanks for getting a new release out 🙂 |
|
Thanks again for the PR and the patience. I fixed a few build system-related issues in this release (hence 0.7.0 rather than 0.6.2) and wanted to test them thoroughly before publishing. |
|
(If you're using the |
|
Sure, happy to have had an excuse to contribute to fluent 🙃 |
I took a stab it. Happy to make changes, if I went off-convention somewhere 🙃
Also tested it in Common Voice and it indeed fixes common-voice/common-voice#986.