-
Notifications
You must be signed in to change notification settings - Fork 0
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
Address TODO - LanguageBehavior is now offered by the generic PropertiesFromAncestorBehavior so use it #2
base: master
Are you sure you want to change the base?
Conversation
slawomir-brzezinski-at-interxion
commented
May 24, 2017
•
edited
Loading
edited
- bonus: thanks to PropertiesFromAncestorBehavior supporting value changes, we can now allow user to see all translations in a single demo, so let's do that. See it in action here (the 'Interface Language' dropdown).
3b6f119
to
ed57882
Compare
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.
Let's try this.
de: { open: "Öffnen", close: "Schließen" }, | ||
es: { open: "Abrir", close: "Cerrar" } | ||
}; | ||
PapyrusDetails = {}; |
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.
Global variables suck. I can't see a reason to change 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.
Well. Global variables are not verboten. Global variable here is just an easy way to access things can be made available before any element is created, where the usage [requires it]/[would benefit from it]. By correctly representing scopes of variance, they aid in understanding complexity and encapsulating variance - if something exists with intention to be always same for all instances, there's no reason it should pollute the properties of each instance, where a user could break these intentions.
Even Polymer prescribes using global variables, not only for behaviors, but for elements as well, fore example to have a custom constructor.
Similarly, global variables are natural for usages where components are declared as vanilla JS classes, (arguably, where Polymer is heading) since a named class declaration essentially creates a variable of that name. Using ES6 module with the class as 'default export' could save from this, but it's only an option. And with ES6 classes, IMO best way to represent this would be a static get
, just like the properties declaration in Polymer 2.0, which also don't get copied to instances.
Here, the reason for the global variable is because usages, such as this <container-with-lang-choice>
, benefit from having available the list of supported languages. If we moved PapyrusDetails.resources
anywhere else, like on the instance of element, we will need rewrite this pretty simple code to something harder to understand. I'm keen to see how such an implementation would stack up.
papyrus-details.html
Outdated
behaviors: [ LanguageBehavior ], | ||
behaviors: [ | ||
PropertiesFromAncestorBehavior({ | ||
lang: { defaultValue: 'en' }, |
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.
I'm a bit concerned about overwriting the web standard lang
property with our own version of it.
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.
I see. HTML elements do have a lang
property.
Well, calling it 'lang' here is exactly deliberate because we explicitly want it to get its value from the very standard attribute, so we have a little conflict of intentions.
I understand the concern about overriding a property, but I haven't found an issue with declaring it in Polymer so far, so let's think of one.
I actually have a feeling this might be benign, at least in most cases, because the only problematic situation I can think of, is for when we want to listen to the changes from such native properties (Polymer won't be able to receive direct sets, hence it needs the clue of ...::eventName
). Here, the native property is overridden only to set its value, as we don't support being able to change the language from the descendant.
ee61cbe
to
dfb1e1b
Compare
…iesFromAncestorBehavior so use it + bonus: thanks to PropertiesFromAncestorBehavior supporting value changes, we can now allow user to see all translations in a single demo, so let's do that
dfb1e1b
to
3df8f5a
Compare