Drag validation #32

danielbeeke · 2017-04-11T20:33:14Z

d.org issue: https://www.drupal.org/node/2868539

In this commit are a settings form, a bit of preprocessing data-attributes and a little proof of concept javascript. The javascript is not yet bug free. I am still trying to wrap my mind around how the tabledrag things work.

What are the opinions about the approach? What needs to be changed?

tonystar · 2017-04-12T13:17:48Z

For reference — here is how Bricks settings looks like:

tonystar · 2017-04-12T13:34:45Z

I am still trying to wrap my mind around how the tabledrag things work.

I'm still trying to understand it! Tabledrag is kind of big black box =)))

tonystar · 2017-04-12T13:49:55Z

Initial review is completed. Conceptually it's very good! Configuration form looks clear and straightforward. And I can confirm that nesting validation is working (with some bugs, but working)!

Bug 1:

Go to Bricks settings > enable Text inside Columns.
Create a new node, add Columns and Text inside it. Validation works.
Add second Text brick and try to drag inside Columns => no success.

danielbeeke · 2017-04-14T09:04:41Z

I have been working on this, it is a bit difficult,
will try to work on it more this weekend

tonystar · 2017-04-14T09:27:35Z

@danielbeeke sure- no pressure!

danielbeeke · 2017-04-15T14:17:07Z

It is a bit better now, but still one ugly bug that I can not find.

https://youtu.be/QCYEYjBb0ps

tonystar · 2017-04-15T15:56:27Z

It's much, much better!!! Main validation functionality works for me.

I reproduced the same bug in a more simple way: https://youtu.be/cthjwseTQmM. Probably this can help. Also, some ideas:

Here you allow to nest under prev non-leaf row:

// Do not go deeper than as a child of the previous row.
maxIndent = $prevRow.find('.js-indentation').length + ($prevRow.is('.tabledrag-leaf') ? 0 : 1);

So if the following conditions will be skipped => we can have a bug.

Comparing to previous row may be not enough. Like on my YouTube video - any parent of previous row is also a candidate (it's probably related to // TODO this may need more recursive like code.).

danielbeeke · 2017-04-18T19:31:36Z

https://www.youtube.com/watch?v=v4VIbKz3wKg&feature=youtu.be&t=34

I still got one very strange bug.
The link starts at the right point, before that I demonstrate that things work fine. On 34 seconds I try to move a group out of its parent. It gets blocked by the first child of the outer parent.

Strange!

tonystar · 2017-04-12T13:22:08Z

bricks.libraries.yml

@@ -1,3 +1,12 @@
 tabledrag.relationship-all:
 js:
 js/tabledrag.relationship-all.js: {}
+
+nesting-validation:


This validation is:

for tabledrag

Bricks-specific

=> so I think a better name would be tabledrag.bricks-validation or just tabledrag.bricks (can be used further for over UI stuff).

tonystar · 2017-04-12T13:26:10Z

bricks.links.menu.yml

@@ -0,0 +1,5 @@
+bricks.settings:
+ title: 'Bricks settings'
+ parent: system.admin_config_media


How about system.admin_structure? Imho, more obvious (like ECK, View modes, Paragraphs etc).

tonystar · 2017-04-12T13:27:13Z

bricks.module

+/**
+ * Returns the nesting settings in a format that the javascript can handle.
+ */
+function bricks_get_nesting_data($return_entity_type = NULL) {


_bricks_get_nesting_data?

tonystar · 2017-04-12T13:29:40Z

bricks.module

@@ -187,6 +187,9 @@ function _bricks_preprocess_tabledrag_form(&$variables, $element_key, $widget, $
 if ($key !== 'add_more') {
 $depth = $element[$key]['depth']['#value'];

+ $variables['table']['#rows'][$key]['data-depth'] = $depth;


Why do we need to store depth as data-attribute? We already have hidden input with depth and can get value right from there! https://github.com/highweb/drupal-bricks/blob/8.x-1.4/bricks.module#L234

tonystar · 2017-04-12T13:32:28Z

bricks.routing.yml

@@ -0,0 +1,7 @@
+bricks.config:
+ path: '/admin/config/media/bricks'


/admin/structure/bricks?

tonystar · 2017-04-12T13:44:05Z

js/nesting-validation.js

+ var prevBundle = $(prevRow).attr('data-bundle');
+ var parentBundle = $(this.group[0]).prev('tr').attr('data-bundle');
+
+ if ($.inArray(prevBundle, settings.bricks.nesting[currentBundle]) !== -1 || $.inArray(parentBundle, settings.bricks.nesting[currentBundle]) !== -1) {


Is currentBundle nestable under prevBundle? => increment values.max. Why to care about parentBundle?

Also: not sure but what if this code is running for say depth 3? Shouldn't values.max equal 3+1?

And what if there is no parent? (depth = 0, delta = 3 in example)

danielbeeke · 2017-04-23T19:51:28Z

I have been working on this, but it seems more difficult.
Will work on it more this week.

I am also replacing the validSwap function

tonystar · 2017-04-26T19:49:14Z

@danielbeeke the code looks much better! Are we still have some critical bugs or must-have refactoring?

tonystar · 2017-06-04T20:29:26Z

@danielbeeke the implementation seems to be not that straightforward and simple…

I suggest the following:

Simplify the logic in Bricks core. In example we can rely on tabledrag-leaf behavior and allow to configure which bundles are "leafs" (ie Images, Text etc). This gives us ~80% of use cases covered with a minimal code.
Wrap everything else from this PR in a separate contrib module. Say, Bricks Restrictions. For complex setups. And it can be updated independently from core.

What do you think?

tonystar reviewed Apr 19, 2017

View reviewed changes

tonystar force-pushed the 8.x-1.x branch from e0a2066 to 525a0fa Compare June 12, 2017 14:43

danielbeeke closed this Mar 19, 2018

danielbeeke force-pushed the 8.x-1.x branch from 26a1944 to af30395 Compare March 19, 2018 18:05

Applying the dynamic entity reference tree inline widget

5c9ae2e

tonystar reopened this Mar 19, 2018

Removing PATCHES file created by composer

a95b6f8

tonystar closed this Mar 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drag validation #32

Drag validation #32

danielbeeke commented Apr 11, 2017

tonystar commented Apr 12, 2017 •

edited

Loading

tonystar commented Apr 12, 2017 •

edited

Loading

tonystar commented Apr 12, 2017 •

edited

Loading

danielbeeke commented Apr 14, 2017

tonystar commented Apr 14, 2017

danielbeeke commented Apr 15, 2017

tonystar commented Apr 15, 2017

danielbeeke commented Apr 18, 2017

tonystar Apr 12, 2017

tonystar Apr 12, 2017

tonystar Apr 12, 2017

tonystar Apr 12, 2017

tonystar Apr 12, 2017

tonystar Apr 12, 2017

danielbeeke commented Apr 23, 2017

tonystar commented Apr 26, 2017

tonystar commented Jun 4, 2017

		@@ -0,0 +1,7 @@
		bricks.config:
		path: '/admin/config/media/bricks'

Drag validation #32

Drag validation #32

Conversation

danielbeeke commented Apr 11, 2017

tonystar commented Apr 12, 2017 • edited Loading

tonystar commented Apr 12, 2017 • edited Loading

tonystar commented Apr 12, 2017 • edited Loading

danielbeeke commented Apr 14, 2017

tonystar commented Apr 14, 2017

danielbeeke commented Apr 15, 2017

tonystar commented Apr 15, 2017

danielbeeke commented Apr 18, 2017

tonystar Apr 12, 2017

Choose a reason for hiding this comment

tonystar Apr 12, 2017

Choose a reason for hiding this comment

tonystar Apr 12, 2017

Choose a reason for hiding this comment

tonystar Apr 12, 2017

Choose a reason for hiding this comment

tonystar Apr 12, 2017

Choose a reason for hiding this comment

tonystar Apr 12, 2017

Choose a reason for hiding this comment

danielbeeke commented Apr 23, 2017

tonystar commented Apr 26, 2017

tonystar commented Jun 4, 2017

tonystar commented Apr 12, 2017 •

edited

Loading

tonystar commented Apr 12, 2017 •

edited

Loading

tonystar commented Apr 12, 2017 •

edited

Loading