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:
What do you think of the suggestions?
Parent
#1260690: META: Improve multilingual user experience in Drupal 8
Comment | File | Size | Author |
---|---|---|---|
#41 | language-add-usability-40.patch | 20.12 KB | Gábor Hojtsy |
#40 | language-add-usability-40.patch | 20.12 KB | Gábor Hojtsy |
#35 | language-add-usability-35.patch | 21.95 KB | Gábor Hojtsy |
#26 | language-add-usability-26.patch | 30.59 KB | Gábor Hojtsy |
#25 | language-add-usability-25.patch | 27.26 KB | Gábor Hojtsy |
Comments
Comment #1
Gábor HojtsyThis 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):
Complex custom language only shown if needed (vs. above):
Comment #3
Gábor HojtsyWe 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?
Comment #4
Gábor HojtsyHere is the add language screen with the fieldset for Custom language back. IMHO looks better. The required field problem still exists.
Comment #5
sun1) 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:
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.
Comment #6
Gábor Hojtsy@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.
Comment #8
Gábor HojtsyUpdated 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.
Comment #10
Gábor HojtsyLooked for affected tests and hopefully fixed them all. Let's see this one.
Comment #12
Gábor HojtsyForgot to save some of the locale.test changes, meh. Rerolled with those.
Comment #14
Gábor Hojtsy#12: language-add-usability-v5.patch queued for re-testing.
Comment #15
sunSeparate line, plz? :)
Missing comma.
Move into the declaration of 'custom_language' ?
Comment #16
Gábor HojtsyFixed 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.
Comment #17
sunThe 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.
Comment #18
Gábor HojtsyThat's true. Understood now. Any further improvement suggestions for this patch or its good to go? :)
Comment #19
Gábor HojtsyTalked 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.
Comment #20
Gábor HojtsyDiscussed 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.
Comment #21
Gábor HojtsyBTW the patch did not apply anymore, here is an interim reroll for the testbot :)
Comment #22
Gábor HojtsyHere 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.
Comment #24
sunI 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.
Comment #25
Gábor HojtsyAll 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.
Comment #26
Gábor HojtsyComponentized 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?
Comment #34
sunPotential usability issue here: The language list is very very long, and "Custom language" only appears at the very end...
Obsolete artifact?
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.
Let's use a regular form submission handler signature here.
-10 days to next Drupal core point release.
Comment #35
Gábor HojtsyHere 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).
Comment #36
sunok, looks good to go for me now.
However, I think it would make to get #1260860: Rework language list admin user interface in first.
Comment #37
Dries CreditAttribution: Dries commentedI committed #1260860: Rework language list admin user interface. Maybe we should ask for a retest?
Comment #38
Dries CreditAttribution: Dries commented#35: language-add-usability-35.patch queued for re-testing.
Comment #40
Gábor HojtsyIronically, 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.
Comment #41
Gábor HojtsyPosting for retest since core testing had issues.
Comment #42
sunComment #43
Gábor HojtsyTrying to stop this sitting around since it blocks other things... Let's get this in!
Comment #44
Gábor HojtsyComment #45
catchCommitted and pushed, thanks!
Comment #47
Gábor HojtsyTagging for base language system.