The add language screen looks pretty bad. Its essentially a simple dropdown most of the cases, but it has lots of baggage that we can get rid of. Some notes from discussions on this page in the Montreal sprint and elsewhere:

- "Predefined language" is very techy. Can we just say pick a language?
- Custom language is only needed if languge is not found in dropdown. We can make a Custom language option appear in the dropdown which could conditionally show the custom language fields. Otherwise no need to bother the user with that. In essense we can automate most of what the help text describes.
- The remaining of the help text is about language codes, which are already clearly explained on the language code field description, so we can get rid of that.

Here is the current screen:

AddLanguage.png

What do you think of the suggestions?

Parent

#1260690: META: Improve multilingual user experience in Drupal 8

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
158.54 KB
61.74 KB
6.4 KB

This is how it would look like. The patch I have really just implements the looks, not the behavior yet (many errors on form validation), still in progress. Reviews welcome nonetheless.

Simpler language add screen (vs. the above):

LanguageAdd.png

Complex custom language only shown if needed (vs. above):

LanguageAddCustom.png

Status: Needs review » Needs work

The last submitted patch, language-add-usability.patch, failed testing.

Gábor Hojtsy’s picture

We might want to keep the fieldset to appear on "Custom language..." to make it appear as one set of fields. Also, the real technical problem here is that there are required fields in the dependent fields. So we either make them non-required on the UI and add custom validation to make them required or alter the form on submission to make them non-required (this I did not test) or make the form multi-step. What sounds best in terms of UX/approach?

Gábor Hojtsy’s picture

Here is the add language screen with the fieldset for Custom language back. IMHO looks better. The required field problem still exists.

CustomLanguage.png

sun’s picture

Status: Needs work » Needs review

1) I'm not sure whether the fieldset is really required. Without the wonky overlay, #1 looks simpler and more appealing than #4.

2) We don't have a concept for conditional, but required form input elements in core yet. Due to that, I'd go with this simple workaround/hack:

$form['custom_language']['#after_build'][] = 'locale_language_add_form_after_build';

function locale_language_add_form_after_build($element, &$form_state) {
  if ($form_state['process_input'] && $form_state['values']['language'] === 'custom') {
    $element['#access'] = FALSE;
  }
  return $element;
}

3) As seen in above example, the form constructor function name should be locale_language_add_form() (singular).

4) Also, mayhaps rename 'predefined_langcode' to 'language'.

5) ick @ _locale_languages_common_controls()... ideally, we'd just have a single locale_language_form() that's re-used and minimally adjusted for the "add" case. Perhaps a separate issue, not sure.

Gábor Hojtsy’s picture

@sun: Thanks for the review. Will address most of your points in a followup. Short comment for now:

1: Its a very easy change in the code either way, I'm not strong on either.

2: I assume you meant !== 'custom', thanks for this great tip!

3: Agreed, although this will introduce some inconsistency for now with other form functions in locale. We can fix them elsewhere.

4: That debate is still going on in #1216094: We call too many things 'language', clean that up; this is a language code input, we call it a langcode consequently. There are two langcode fields on the form, the manual one is called plainly "langcode" since it is a field shared with the regular editing form. Therefore this one needs to be langcode_something or something_langcode as far as I see. We can call it language, just like in the schema, but that would further blur the what we call a language vs. what we call a langcode which we don't seem to ever get agreement on :)

5: I think a followup would be great. With #1280530: Decouple domain/path negotiation setup from language configuration this form is getting much simpler, so it is going to be easier to integrate with others.

Status: Needs review » Needs work

The last submitted patch, language-add-usability-v2.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
15.97 KB

Updated patch. Changes:

- renamed all locale_languages_* to locale_language_* since the add rename affected other renames and its best to just do it then alltogeher IMHO
- added an after_build callback; unfortunately setting #access = FALSE does not work since the #access propagation code runs before the after_build code; setting values to #required FALSE did not work since there are other validators running still; so "unsetting" the element by returning an empty array was the most straightforward way to do it
- back to using a container, not a fieldset

That's it. I think this addresses all your feedback except renaming predefined_langcode to language, which I've discussed above. custom_language is named so because its a container for all the properties of a language.

Status: Needs review » Needs work

The last submitted patch, language-add-usability-v3.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
23.82 KB

Looked for affected tests and hopefully fixed them all. Let's see this one.

Status: Needs review » Needs work

The last submitted patch, language-add-usability-v4.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
30.63 KB

Forgot to save some of the locale.test changes, meh. Rerolled with those.

Status: Needs review » Needs work
Issue tags: -Usability, -D8MI

The last submitted patch, language-add-usability-v5.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +D8MI

#12: language-add-usability-v5.patch queued for re-testing.

sun’s picture

Status: Needs review » Needs work
+++ b/modules/locale/locale.admin.inc
@@ -153,51 +153,43 @@ function locale_languages_overview_form_submit($form, &$form_state) {
+  $form['predefined_langcode'] = array('#type' => 'select',

Separate line, plz? :)

+++ b/modules/locale/locale.admin.inc
@@ -153,51 +153,43 @@ function locale_languages_overview_form_submit($form, &$form_state) {
+    '#states' => array(
+      'visible' => array(
...
+      )

Missing comma.

+++ b/modules/locale/locale.admin.inc
@@ -153,51 +153,43 @@ function locale_languages_overview_form_submit($form, &$form_state) {
+  $form['custom_language']['#after_build'][] = 'locale_language_add_form_after_build';

Move into the declaration of 'custom_language' ?

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
30.63 KB

Fixed all those.

Strange stuff though :) You say $form['actions'] and $form['actions']['submit'] should be separate (at http://drupal.org/node/1260860#comment-5077964 - number 2), while #after_build is best integrated into a giant nested array structure. Personally I prefer the giant nested array structures, so I'm all for using them.

This patch should fix all your concerns. No other changes so should still pass the tests.

sun’s picture

The common sense we're following and applying is: Declare one element at once.

#type actions is a container element, #type submit is a separate button element. #after_build is just a property on a container/ex-fieldset/tree-structure.

Gábor Hojtsy’s picture

That's true. Understood now. Any further improvement suggestions for this patch or its good to go? :)

Gábor Hojtsy’s picture

Talked to yoroy about a UX review. He said since we are rejuggling these forms here and in #1301148: Stop pretending we have configuration translation for languages and #1280530: Decouple domain/path negotiation setup from language configuration, we should take a deeper look at it once we have those in, so we can clear up the text for the remaining / reorganized fields. He was very happy with the simplified experience that results from the three patches (this one and the mentioned two) combined.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Discussed our after_build trick with @chx. After exploring several options, we arrived at the conclusion that at the core we need a form structure changed based on the select option, and that Drupal only has one elegant solution for this: Ajax. That changes both the displayed and server side form structure. Moving there would need a more significant rework of this patch though.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
30.38 KB

BTW the patch did not apply anymore, here is an interim reroll for the testbot :)

Gábor Hojtsy’s picture

Here is the AJAX version. It looks like the same overall. I have two issues though:

1. When you choose a different select item from the predefined dropdown, the throbber shows which makes the submit button jump too. Do we need custom one-of styling for this form to fix that?

2. The no-JS experience is not good. You can add predefined languages, but if you choose custom, obviously the form items will not yet appear. Then you need to press "Add language" (kinda strange) at which point I'd expect the form to show up with validation errors (which would be sort of strange too). However, the form is not rebuilt and validated with the new structure as it seems and you get a crappy empty langcode language added. That's not good. Looking for advice on how to overcome this problem and get the form rebuilt in the non-JS case at least.

Status: Needs review » Needs work

The last submitted patch, language-add-usability-ajax-22.patch, failed testing.

sun’s picture

I don't think we need AJAX for this. Simply:

1) rename the current submit button to "Add custom language".

2) add a second submit button next to the select, which says "Add language".

3) add #limit_validation_errors to aforementioned "Add language" button in 2) to only validate itself and ignore the custom language values.

4) add #states to aforementioned "Add language" button in 2) to hide the button if the "Custom language" option is selected in the dropdown list.

5) add a global form validation handler or #element_validate to aforementioned "Add language" button in 2) to throw a form error when a user attempts to submit the form with "Add language" button but has "Custom language" selected.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
27.26 KB

All right, doing the testing / form rebuilds with Ajax got a bit hard anyway ;). Here is a patch with this approach then. Not yet implemented 5) fully since we need to componentize the validation handler for predefined_langcode then so they still run on #limit_validation_errors. Works remarkably well with JS on and maybe even passes tests, will see.

Gábor Hojtsy’s picture

Componentized form validation. and submission and now it works consistent, the buttons only work with their respective form sections and #states does the magic for JS enabled sites. Good now?

Status: Needs review » Needs work
Issue tags: -Usability, -D8MI

The last submitted patch, language-add-usability-26.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, language-add-usability-26.patch, failed testing.

Status: Needs review » Needs work
Issue tags: +Usability, +D8MI

The last submitted patch, language-add-usability-26.patch, failed testing.

sun’s picture

Status: Needs review » Needs work
+++ b/modules/locale/locale.admin.inc
@@ -153,51 +153,53 @@ function locale_languages_overview_form_submit($form, &$form_state) {
+function locale_language_add_form($form, &$form_state) {
+  $predefined_languages = _locale_prepare_predefined_list();
+  $predefined_languages['custom'] = t('Custom language...');

Potential usability issue here: The language list is very very long, and "Custom language" only appears at the very end...

+++ b/modules/locale/locale.admin.inc
@@ -153,51 +153,53 @@ function locale_languages_overview_form_submit($form, &$form_state) {
+function locale_language_add_form_ajax($form, $form_state) {
+  return $form['custom_language'];
 }

Obsolete artifact?

+++ b/modules/locale/locale.admin.inc
@@ -283,56 +285,77 @@ function _locale_languages_common_controls(&$form, $language = NULL) {
+    form_set_error('predefined_langcode', t('Fill in the language details and save the language with <em>Add custom language</em>.'));
...
+    form_set_error('predefined_langcode', t('Use the <em>Add language</em> button to save a predefined language.'));
   }

I usually separate the string of the button label into a replacement token in errors/messages like this, so it is guaranteed that the message contains the identical string/translation.

+++ b/modules/locale/locale.admin.inc
@@ -283,56 +285,77 @@ function _locale_languages_common_controls(&$form, $language = NULL) {
+function locale_language_add_finalize_submit($langcode, &$form_state) {

Let's use a regular form submission handler signature here.

-10 days to next Drupal core point release.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
21.95 KB

Here is an updated patch. Re your comments:

1. Well, the point is that people figure out they do need to fill in a custom form is when they scanned the list of available languages and did not find theirs. I have #1295696: Sync predefined list of languages with localize.drupal.org and extend native language information taking care of limiting the list to a more sane set with languages that people actually use Drupal in.

2. Removed.

3. I don't think we should start a debate here on that, and this is holding down major progress elsewhere. I don't think Drupal core does that.

4. As per our IRC discussion I've changed that to be a utility function and renamed to ..._add_set_batch() and included $langcode only moving the form_state change to both submission handlers respectively.

+1. As per our IRC discussions I've also removed all changes for locale_languages_ => locale_language since it was too big of a change and too unrelated to this patch. So all the new functions currently follow locale_languages_* too for consistency. Never be concerned, since #1301040: Move language listing functionality from locale.module to a new language.module is around the corner and going to do massive function renames along the lines of locale_languages_* => language_* and locale_language_* => language_*, so it is going to be all right soon. Promise.

Wondering how this stacks up now. Since there is so much work invested into this already, its holding up more work on #1301040: Move language listing functionality from locale.module to a new language.module (among a few other issues listed there, see http://drupal.org/node/1301040#comment-5107388, so would be great to wrap this up and get in to move on with more exciting stuff).

sun’s picture

Status: Needs review » Reviewed & tested by the community

ok, looks good to go for me now.

However, I think it would make to get #1260860: Rework language list admin user interface in first.

Dries’s picture

I committed #1260860: Rework language list admin user interface. Maybe we should ask for a retest?

Dries’s picture

Issue tags: -Usability, -D8MI

#35: language-add-usability-35.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Usability, +D8MI

The last submitted patch, language-add-usability-35.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
20.12 KB

Ironically, none of the hunks that contributed to "failed to apply" were due to #1260860: Rework language list admin user interface but rather other patches that changed the locale_help() hook and independently fixed whitespace in search.test. Here is an updated patch, no changes whatsoever, just a reroll.

Gábor Hojtsy’s picture

Posting for retest since core testing had issues.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Gábor Hojtsy’s picture

Assigned: Unassigned » Dries

Trying to stop this sitting around since it blocks other things... Let's get this in!

Gábor Hojtsy’s picture

Assigned: Dries » Unassigned
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed, thanks!

Status: Fixed » Closed (fixed)

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

Gábor Hojtsy’s picture

Issue tags: +language-base

Tagging for base language system.