Pardon me if there is already an issue for this, but I tried to search first and came up with nothing.

I am kind of annoyed by the way we are saving translated strings on the translation page. And this might apply elsewhere, in which case, please provide more info. But the actual problem is this:

- I go to the "Translate interface" page to translate one specific string.
- I find it, enter some text in the translation field, and hit the save button.
- I expect the form to save, but instead there is now a notice telling me that "Changes made in this table will not be saved until the form is submitted." which in turn pushed the submission button down. Which is kind of a paradox, since I thought I just submitted the form, but the button never got clicked, because I needed to be informed about the importance of submitting. :)

Possibly best explained with a gif (as everything else in life) :). Attached

So, my rant is now over.

The problem here is that the inserting of a notice pushes the DOM down before the click is propagated to the button. Or more precisely, the event that triggers the change happens before the event that is the click. So the easy fix is the following:

- Use a small setTimeout on the showing of the notice. This way the click would be allowed to happen before we push the DOM downwards.

I have written a lot of javascript over the years, but I am not familiar with styles and guidelines (and libraries available) for Drupal core. Maybe we can utilze something there. Also, I am using a named anonymous function (for easier debugging) and _this for making sure we have the right _this. Not sure if that is within style? At least it passes eslint.

Ok. so patch coming up at least. Not just ranting

CommentFileSizeAuthor
#11 2471136-11.patch1.65 KBeiriksm
#5 2471136-5.patch1.11 KBeiriksm
#4 2471136-4.patch1.11 KBeiriksm
#2 d8-form-change-ux-after.gif222.45 KBeiriksm
#1 2471136.patch1.14 KBeiriksm
d8-form-change-ux.gif120.33 KBeiriksm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
Status: Active » Needs review
FileSize
2471136.patch1.14 KB
eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
FileSize
d8-form-change-ux-after.gif222.45 KB

Also, since I am already advocating that everything is best explained in gifs. Here is the after gif.

I might add, that this approach makes it so that if you click the save button and hold it, your button will release when the DOM has pushed down, and the same behaviour will show itself again. You will have to click a second time.

googletorp’s picture

googletorp CreditAttribution: googletorp commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/locale/locale.admin.js
@@ -11,9 +11,15 @@
+          var _this = this;

    I believe best practice is to use the variable "that" to refer to this in a different scope. 

  2. +++ b/core/modules/locale/locale.admin.js
@@ -11,9 +11,15 @@
+          setTimeout(function showLocaleTranslateWarning () {

    Should be anonymous function.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
Status: Needs work » Needs review
FileSize
2471136-4.patch1.11 KB

Thanks for reviewing. Fair points.

I believe best practice is to use the variable "that" to refer to this in a different scope.

I would argue it is somewhat a matter of taste, and of course if there is an established style, that should be followed.

In Drupal core js, it seems to be two different paradigms:

var that = this;

and

var form = this; // Or whatever _this_ would be referring to. In the case of this patch, I guess I could use var table = this;

I have also seen stuff like var self = this (in core) and formObject = this (instead of form = this). As a matter of fact, in one case, there is a file that has both "var that = this;" and then 60 lines down: "var self = this;". So best practice or style, none of them are really consistent. But point is, no occurences of _this, as I used in my patch. So that is a very good point, either way. So I changed it to "that" anyway.

Updated patch with your suggestions. Thanks again for reviewing.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
FileSize
2471136-5.patch1.11 KB

Heh, sorry. Was a bit quick with the changing of _this :)

googletorp’s picture

googletorp CreditAttribution: googletorp commented
Status: Needs review » Reviewed & tested by the community

Looks good.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Reviewed & tested by the community » Needs work

unfortunately the js isn't up to core standards. let me do a proper review it needs work.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

What could be done with minimal code changes it to use debouce (or throttle really), can you try something like:

Changing

        $form.one('change.localeTranslateDirty', 'table', function () { });

to 

        $form.one('change.localeTranslateDirty', 'table', Drupal.debounce(function () { }));

And that should work (a dependency on the core/drupal.debouncelib would need to be added as well).

googletorp’s picture

googletorp CreditAttribution: googletorp commented

#8 It seems like a hack to me to use debounce for this. This is not the purpose for debounce and since debouce uses setTimeout anyways it doesn't really make any sense to me.

I have a hard time understanding why adding extra unneeded scripts and complexity should be more "up to core standards", as you put it yourself.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

You're right, it's a dirty hack. Looked into it, we should be using our custom formUpdated event instead of change.

We need to add an explicit dependency on core/drupal.form. The file is already there because of something else on the page so we're not adding unrelated stuff we're just being explicit about it.

Changing:
$form.one('change.localeTranslateDirty', 'table', function () {});
to
$form.one('formUpdated.localeTranslateDirty', 'table', function () {});

Same for the other event below.

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
CreditAttribution: eiriksm at Ny Media AS commented
Status: Needs work » Needs review
FileSize
2471136-11.patch1.65 KB

Using drupal/form works.

Updated patch.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Reviewed & tested by the community

This works as expected. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed affe3f8 and pushed to 8.0.x. Thanks!

  • alexpott committed affe3f8 on 
    Issue #2471136 by eiriksm, nod_: Improve user interface for translating...
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Issue tags: +D8MI, +language-ui

Awesome, thanks a lot! Tagging for statistics.

Status: Fixed » Closed (fixed)

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