Be consistent with respect to which strings within JS code should be overriddable via drupalSettings

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.

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.

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.

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

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

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

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.

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.

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

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.

Agreed with @effulgentsia.

I will try this one ;)

Hope it's fine :)

Deleted some unused variable assignments like:

var strings = this.options.strings

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

ucfirst

Capitalized strings ;)

In my queue to review.

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.

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

It seems ok. Let's commit it :)

Committed and pushed to 8.x. Thanks!

Yay!