Problem/Motivation

It's a bit related to our flattening issue, but much more simple for consistency:
Flattening: #2825577: [META] Optional UI flattening

Users can drag & drop when editing the translation.
This can cause confusion because it affects all other languages.

Proposed resolution

Now that we have consistency expectations of the UI, we should apply it consequently:
#2923301: Widget actions hook doesn't have information about translating status

We should check allowReferenceChanges and only display the drag & drop handler if the reference should be updated.

User interface changes

Consistently hiding actions that affect more than just the translation.

API changes

Data model changes

CommentFileSizeAuthor
#24 Screenshot from 2018-01-22 12-07-55.png24.76 KBjohnchque
#24 Screenshot from 2018-01-22 12-08-02.png15.88 KBjohnchque
#24 interdiff-2932334-23-24.txt1.12 KBjohnchque
#24 hide_drag_handler-2932334-24.patch5.88 KBjohnchque
#23 Screenshot from 2018-01-22 10-45-34.png10.82 KBjohnchque
#23 interdiff-2932334-17-23.txt1.22 KBjohnchque
#23 hide_drag_handler-2932334-23.patch5.47 KBjohnchque
#22 paragraph-item-missing.png16.02 KBmiro_dietiker
#20 Screenshot from 2018-01-19 08-57-24.png40.23 KBjohnchque
#20 Screenshot from 2018-01-19 08-57-31.png22.97 KBjohnchque
#20 Screenshot from 2018-01-19 08-59-04.png24.33 KBjohnchque
#17 hide_drag_handler-2932334-17.patch4.61 KBjohnchque
#17 hide_drag_handler-2932334-17-test-only.patch3.07 KBjohnchque
#13 interdiff-2932334-10-13.txt4.33 KBjohnchque
#13 hide_drag_handler-2932334-13.patch4.67 KBjohnchque
#13 hide_drag_handler-2932334-13-test-only.patch3.13 KBjohnchque
#11 interdiff-2932334-9-10.txt2.5 KBjohnchque
#11 hide_drag_handler-2932334-10.patch3.93 KBjohnchque
#9 hide_drag_handler-2932334-9.patch1.26 KBjohnchque
#4 hide_drag_handler-2932334-4.patch2.1 KBjohnchque

Comments

miro_dietiker created an issue. See original summary.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Parent issue: » #2932344: [META] Hide shared actions when translating
miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

At its core this is now about checking $this->allowReferenceChanges()... But we need to inject the information to the field items template, likely override it.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Active » Needs review
StatusFileSize
new hide_drag_handler-2932334-4.patch2.1 KB

Maybe not the best way but it works. :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

That's pretty nasty :)

This replaces it globally, but we still want it for other/nested multi-value fields even when translating.

I would have expected that we can find a way to kick out the #tabledrag settings that paragraphs_preprocess_field_multiple_value_form() that template_preprocess_field_multiple_value_form() added, that might be enough? Might keep weight fields visible or so though, so we possibly want to explicitly hide them.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Also, the test you changed is not a JS test, so it will never see elements that are just added by JS. If we add a test to make sure those elements are hidden, we also need to make sure they are visible when they should be.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work
johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Assigned: Unassigned » johnchque

Gonna try that.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs work » Needs review
StatusFileSize
new hide_drag_handler-2932334-9.patch1.26 KB

Working on tests. :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/paragraphs.module
@@ -341,6 +341,16 @@ function paragraphs_preprocess_field_multiple_value_form(&$variables) {
+        unset($variables['table']['#rows'][$key]['data'][2]);

hm, this is interesting, I would expect we still need to know the delta but it looks like nothing is failing..

Maybe we could revert what the default preprocess is doing (move it out of $item) and add an #access FALSE?

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
StatusFileSize
new hide_drag_handler-2932334-10.patch3.93 KB
new interdiff-2932334-9-10.txt2.5 KB

Learning JS testing. :)

Status: Needs review » Needs work

The last submitted patch, 11: hide_drag_handler-2932334-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs work » Needs review
StatusFileSize
new hide_drag_handler-2932334-13-test-only.patch3.13 KB
new hide_drag_handler-2932334-13.patch4.67 KB
new interdiff-2932334-10-13.txt4.33 KB

This should work. Updated tests. :)

The last submitted patch, 13: hide_drag_handler-2932334-13-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 13: hide_drag_handler-2932334-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented 
+++ b/tests/src/FunctionalJavascript/ParagraphsExperimentalWidgetElementsTest.php
@@ -0,0 +1,92 @@
+    $this->createScreenshot('/home/john/Documents/screenshot.jpg');

Testbot doesn't like this... ;-)

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs work » Needs review
StatusFileSize
new hide_drag_handler-2932334-17-test-only.patch3.07 KB
new hide_drag_handler-2932334-17.patch4.61 KB

:facepalm: my bad. Fixed now.

The last submitted patch, 17: hide_drag_handler-2932334-17-test-only.patch, failed testing. View results

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

This is a UI change. Please provide some screenshots. :-)
Preferrably with multiple nesting levels, once expanded, once mostly collapsed.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs work » Needs review
StatusFileSize
new Screenshot from 2018-01-19 08-59-04.png24.33 KB
new Screenshot from 2018-01-19 08-57-31.png22.97 KB
new Screenshot from 2018-01-19 08-57-24.png40.23 KB

Right, so the changes are:

Translation screen without the patch.

Translation screen with the patch.

Node edit screen with the patch.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Issue tags: +Needs usability review, +Usability

OK, that still looks nice and the indentation is still pretty clear to the user.
Thus, i consider committing this to make the separation complete...

Any other UX feedback?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Issue summary: View changes
Status: Needs review » Needs work
Related issues: +#2825577: [META] Optional UI flattening
StatusFileSize
new paragraph-item-missing.png16.02 KB

Tested this and while on it i found: #2938452: Untranslatable containers break translation UI consistency

That said from extensive testing: the hierarchy display feels a bit different. Here are my thoughts:

1) Less indentation
This seem to be acceptable. There is enough indentation to recognize it in nesting.
It's already a step into the direction of: #2825577: [META] Optional UI flattening

2) The drag handle was a visual reference point and makes you recognise an item in multi value fields.
Users are used to it. The item horizontal border are not strong enough:

One idea was to indicate nesting depth with a bullet item like this:
-
--
---
--
--
So every child would display a bullet item for itself while it also repeats the parent bullet.
This would lead to more nesting depth awareness.
Removing the indication completely and reducing indentation would be subject of #2825577: [META] Optional UI flattening

While this needs more work in follow-ups, i think we should replace the handle with a single bullet item in this issue.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs work » Needs review
StatusFileSize
new hide_drag_handler-2932334-23.patch5.47 KB
new interdiff-2932334-17-23.txt1.22 KB
new Screenshot from 2018-01-22 10-45-34.png10.82 KB

Right, better to add some kind of indication, replaced the handler by a bullet item during translation.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
StatusFileSize
new hide_drag_handler-2932334-24.patch5.88 KB
new interdiff-2932334-23-24.txt1.12 KB
new Screenshot from 2018-01-22 12-08-02.png15.88 KB
new Screenshot from 2018-01-22 12-07-55.png24.76 KB

Going mobile first. :) These are the results.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Thx, looks nice with the smaller bullets.
I'm a little bit irritated about the huge spacing on mobile. Thought it's a little bit smaller currently. But we can't see the mobile touch expand/collapse button right as it's cutoff.

I pinged Ivica to review the CSS implementation before commit.. :-)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Fixed

Tested again and the vertical spacing is fine. Also the bullet items: Clearly visible and still not too dominant. Fine on mobile and desktop.

Let's get this in for now and then figure out how to improve it. :-)

Status: Fixed » Closed (fixed)

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