If you search for strings: { within HEAD JS files, you'll find several strings declared that way within contextual.js, contextual.toolbar.js, edit.js, and toolbar.js. Being declared this way, they can be overridden via drupalSettings. However, within some of these same JS files, you also have calls to Drupal.t() of strings not declared via the options.strings object.

What is the practical purpose of allowing some strings to be overridden via drupalSettings, and are we choosing the correct ones? Is this a pattern we want to spread to other JS files?

Files: 
CommentFileSizeAuthor
#21 js-strings-2058843-21.patch3.6 KBjessebeach
PASSED: [[SimpleTest]]: [MySQL] 58,647 pass(es).
[ View ]
#19 2058843-19.patch4.98 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 58,394 pass(es).
[ View ]
#17 2058843-17.patch4.98 KBrteijeiro
PASSED: [[SimpleTest]]: [MySQL] 58,803 pass(es).
[ View ]

Comments

effulgentsia’s picture

Issue tags:+JavaScript
jessebeach’s picture

My thinking with this pattern is it provides a means to change strings from PHP by providing configuration settings, rather than mashing the text nodes in the client with direct manipulation or overriding attach methods of modules' JS files.

I've run into a need for this type of overriding when working with Drupal Gardens clients. This isn't a translation issue. I've had clients want to change, for instance, the text of a button from Submit to Post.

webchick’s picture

Typically that would be done by setting up a "Better English" translation and doing it that way. Having two ways of accomplishing the same thing (but only in JS) seems sub-optimal to me.

jessebeach’s picture

We didn't have English-to-English translation when this code overriding pattern was introduced some months ago. I'm cool with cleaning it out in favor of a UI settings solution.

effulgentsia’s picture

I've had clients want to change, for instance, the text of a button from Submit to Post.

"Custom English" (I'll leave out the judgment on whether it's "Better") could "translate" Submit to Post globally, but not for an individual button.

webchick’s picture

Shouldn't that be able to be solved with the t() context system? Does that exist for JS?

effulgentsia’s picture

Yes it does. Drupal.t() receives a options parameter, which can contain a 'context' key.

So then the question becomes when to add that. Here's my thought:

Within contextual.js, we have the strings 'open' and 'close'. Those are generic, so could potentially use a context option, except I think they're only output already in the context of another string, so we should do the same thing there that we did in #2052973: Improve translatability of strings in toolbar.js, and replace Drupal.t('@action @title configuration options') with Drupal.t('Open @title configuration options') and Drupal.t('Close @title configuration options'). Those longer strings then provide sufficient context implicitly so wouldn't need a further Drupal.t() options.context.

Within contextual.toolbar.js, the only currently overridable string that's somewhat generic is 'Press the esc key to exit.'. Does it make sense to add a context for that, or is it more likely that if someone wants to customize that string, they'll want to customize it to the same thing everywhere it's used? Currently, within core, it's only used in this one place, so we have to guess at that answer.

Within edit.js, we have 'Quick edit'. Same question. And same situation that we have no other place it's used within core.

Within toolbar.js, we have 'Horizontal orientation' and 'Vertical orientation'. Same question and same situation.

jessebeach’s picture

As we get more unique strings for aural UIs, we're now treading on UI design and theming, rather than translation.

hass’s picture

I grined a bit when I read "text of a button from Submit to Post"... I hope this is not drupal as we have no submit and post buttons anymore. Buttons must be named what they do e.g "Save configuration". :-)

You could override the js file completly if nothing works for your client...

jessebeach’s picture

Overriding a JS file is an option, but it seems like a nuclear option. We can expose string configuration to settings. The question is should we.

hass’s picture

I think this is the wrong way. Write better and longer strings and you are done or use string override to change an english string. This is not limited to translations.

Gábor Hojtsy’s picture

I don't fully understand the OP. Is this about a parallel string override system additionally to Drupal.t()'s translation support? Drupal 8 includes English to English translation capability (enable Interface translation module and make English translatable by checking a box on the edit page for the English language). There is no need to add a "Better English" language or anything like that anymore. (As it was possible in Drupal 7 by adding a "Better English" language).

Even if you don't have the Interface translation module but want to override strings, the same technique can be used to inject the string array to JS and the Drupal.t() override would work. So not sure we should have a parallel override system at all.

In terms of the specific strings, Drupal.t('Open @title configuration options') is leaps and bounds ahead in terms of translatability vs. Drupal.t('@action @title configuration options'). Just pulling out words like that mostly leads to very bad translatability. It sounds like a code optimization but linguistics are not optimizable like that...

jessebeach’s picture

The reason I like declaring strings at the top of the file is that they are all collected in one place; it's easy to find them without parsing the entire document.

var options = $.extend({
  strings: {
    open: Drupal.t('open'),
    close: Drupal.t('close')
  }
}, drupalSettings.contextual);

Whether or not one chooses to override the strings via JS is irrelevant to the pattern of declaring the strings in one place. It happens that one can override the strings given this pattern, sure. But we have a method now of declaring arbitrary language translations, so that should be the way that folks take to change terms in the UI.

If we have random invocations of Drupal.t in the code, then those strings should be declared in the strings hash on the scoped options variable like the rest. It's really just more of a code cleanliness issue.

effulgentsia’s picture

Whether or not one chooses to override the strings via JS is irrelevant to the pattern of declaring the strings in one place.

I disagree. If we want to declare strings at the top of the JS file, that's great, but we shouldn't allow them to be overridden via drupalSettings, because that will encourage/confuse people into doing so, rather than using translation contexts.

Could we do something like this instead?

var options = $.extend({
}, drupalSettings.contextual);

var strings = {
  open: Drupal.t('open'),
  close: Drupal.t('close')
}

Gábor Hojtsy’s picture

Agreed with @effulgentsia.

rteijeiro’s picture

Assigned:Unassigned» rteijeiro

I will try this one ;)

rteijeiro’s picture

Status:Active» Needs review
StatusFileSize
new4.98 KB
PASSED: [[SimpleTest]]: [MySQL] 58,803 pass(es).
[ View ]

Hope it's fine :)

Deleted some unused variable assignments like:

var strings = this.options.strings

hass’s picture

Status:Needs review» Needs work
+++ b/core/modules/contextual/js/contextual.js
@@ -8,12 +8,13 @@
+  open: Drupal.t('open'),
+  close: Drupal.t('close')

ucfirst

rteijeiro’s picture

Status:Needs work» Needs review
StatusFileSize
new4.98 KB
PASSED: [[SimpleTest]]: [MySQL] 58,394 pass(es).
[ View ]

Capitalized strings ;)

jessebeach’s picture

Assigned:rteijeiro» jessebeach
Issue tags:+sprint

In my queue to review.

jessebeach’s picture

StatusFileSize
new3.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,647 pass(es).
[ View ]

rteijeiro, what do you think about this approach? We can change much less code by simply flipping the order of the arguments passed to $.extend(). So instead of:

var options = $.extend({
    strings: {
      open: Drupal.t('Open'),
      close: Drupal.t('Close')
    }
  },
  drupalSettings.contextual
);

We declare the strings last so they merge left and take precedence.

var options = $.extend(drupalSettings.contextual,
  // Merge strings on top of drupalSettings so that they are not mutable.
  {
    strings: {
      open: Drupal.t('Open'),
      close: Drupal.t('Close')
    }
  }
);

drupalSettings.contextual can be undefined.

var a = $.extend(undefined, {b: 'c'});

will yield

{
  b: 'c'
}

Taking this approach, we don't have to change any code in the Views or Models, just a few variable declarations. It much less risky.

rteijeiro’s picture

Awesome!! Let's wait and see what says the testbot ;)

rteijeiro’s picture

Status:Needs review» Reviewed & tested by the community

It seems ok. Let's commit it :)

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags:-sprint

Yay!

Automatically closed -- issue fixed for 2 weeks with no activity.