Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

andypost’s picture

Status: Active » Needs review
FileSize
10.34 KB
andypost’s picture

FileSize
17.33 KB

Merge patch with generic to test.

Weight does not saved when Save clicked but works for Contact categories list

andypost’s picture

Missed a hook menu change

andypost’s picture

And final patch

jthorson’s picture

Status: Needs work » Needs review

Don't mind the failures on -2, -3, and -4 ... I'm just doing some pruning to shorten up the queue. If you did want independent results from the previous patches, feel free to re-test.

Status: Needs review » Needs work

The last submitted patch, 1891690-voc-list-5.patch, failed testing.

andypost’s picture

andypost’s picture

andypost’s picture

Issue summary: View changes

Added related

andypost’s picture

Status: Postponed » Needs review
FileSize
11.57 KB

I think better to limit the scope of the patch and make clean-up and refactoring in #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)

So introduced on top of FormInterface same as for #1872870: Implement a RoleListController and RoleFormController

andypost’s picture

andypost’s picture

FileSize
10.8 KB

re-roll

twistor’s picture

Status: Needs review » Needs work

Looks good. One major problem though, this adds a new operation, "Disable" to the dropbutton.

@@ -0,0 +1,163 @@
+<?php
+
+/**
+ * Contains \Drupal\taxonomy\VocabularyListController.
+ */

Missing @file.

andypost’s picture

Status: Needs work » Needs review
FileSize
11.24 KB
2.2 KB

nice catch!

Patch also converts this listing to controller that was introduced in #1983844: Add EntityListController and convert picture.module callbacks to routes/controllers to provide a use-case

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyTest.phpundefined
@@ -93,7 +93,7 @@ function testTaxonomyAdminChangingWeights() {
-    $this->drupalPost('admin/structure/taxonomy', $edit, t('Save'));
+    $this->drupalPost('admin/structure/taxonomy', $edit, t('Save order'));

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyListController.phpundefined
@@ -0,0 +1,164 @@
+      '#value' => t('Save order'),

+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -9,88 +9,6 @@
-    $form['actions']['submit'] = array('#type' => 'submit', '#value' => t('Save'), '#button_type' => 'primary');

Why are you changing this string in a conversion issue?

andypost’s picture

Status: Needs work » Needs review
FileSize
10.43 KB
1.43 KB

So back to original. I think "Save order" better describes that but let's leave it for #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Configurables, +FormInterface

Thanks!

twistor’s picture

Status: Reviewed & tested by the community » Needs work

This still has the "Disable" link in the dropbutton.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

@twistor please open an issue to fix that generically, those should not be added unless the ConfigEntity defines a status entity_key.

tim.plunkett’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/VocabularyListController.phpundefined
@@ -0,0 +1,164 @@
+    if (count($entities) > 1) {
+      return drupal_get_form($this);
+    }

This looks unnecessary... and if it is... it certainly needs a comment. I've tested the code without this and it works as expected.

alexpott’s picture

So this adds the ability to drag the rows around to order the weight...

// Creates a form for manipulating vocabulary weights.
andypost’s picture

Status: Needs work » Needs review
FileSize
10.54 KB
679 bytes

Comment added

jibran’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Vocabulary.phpundefined
@@ -29,7 +30,8 @@
+ *     "uuid" = "uuid",

I think , shouldn't be here.

andypost’s picture

FileSize
10.54 KB
517 bytes

sure

amateescu’s picture

I think , shouldn't be here.

On the contrary, it should. This is not JavaScript :) The correct patch is the one from #23.

andypost’s picture

@amateescu no, all entity_keys in core have no last comma. seems it does not matter at all (bot will show)

amateescu’s picture

Oh, that was in the annotation? Sorry @jibran, my bad :/

Status: Needs review » Needs work
Issue tags: -Configurables, -FormInterface

The last submitted patch, 1891690-voc-list-25.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Configurables, +FormInterface

#25: 1891690-voc-list-25.patch queued for re-testing.

podarok’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +CodeSprintUA

#25 looks good for me
lets make it commited

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4370959 and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.