-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
create a README for the LocalizeMixin #102
Conversation
Changes made:
|
mixins/localize-mixin.js
Outdated
} | ||
this._onRequestResponse(res, this.__language); | ||
}); | ||
this.__namespace = `${this.__namespaceBase}:${this.__language}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be this.namespaceBase
(without the double underscores)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it definitely should
Recent changes:
I haven't updated the localize mixin readme yet to reflect these changes in case there is more churn |
mixins/localize-mixin.js
Outdated
this._onRequestResponse(res, this.__language); | ||
this.__language = Object.keys(res)[0]; | ||
this.__resources = {}; | ||
this.__resources[this.__language] = res[this.__language]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ the way I did this was weird. will change.
Any additional feedback on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot simpler than before!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to distract from the original purpose of this PR, but just want to throw something out there...
Currently, the localize mixin adds an observer to the document for each instance where the mixin is used. I'm not sure if this is a concern or not...?
In contrast, the Polymer DirMixin creates a single global observer, and the instances of the mixin add/remove a callback when connected/disconnected. For our Lit RtlMixin
I chose not to do this, keeping in mind that we might add it if we wanted to handle dir
attribute changes after the initial page-load. It's an approach we could take for the localize mixin, but I'm not sure if it's needed.
@dbatiste Agreed. Worth throwing that into an Issue so we don't forget about it? |
Sounds good - thx Margaree... you beat me to it. ;) |
This looks great, lots of nice detail! |
Much of this is copied from the d2l-localize-behavior, with mostly the usage and language resources being updated.