Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

slawomir-brzezinski-at-interxion
Copy link
Collaborator

@slawomir-brzezinski-at-interxion slawomir-brzezinski-at-interxion commented May 24, 2017

  • 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).

Copy link
Collaborator

@timrog1 timrog1 left a 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 = {};
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

behaviors: [ LanguageBehavior ],
behaviors: [
PropertiesFromAncestorBehavior({
lang: { defaultValue: 'en' },
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@slawomir-brzezinski-at-interxion slawomir-brzezinski-at-interxion force-pushed the Use-PropertiesFromAncestorBehavior branch 2 times, most recently from ee61cbe to dfb1e1b Compare June 1, 2017 14:05
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants