Merge "List links" page into "Edit menu" page

This issue might be about a couple things at once, but I'm refocusing the title on what seems like the main one. The bottom line is that these two tabs confuse just about everyone, and we should do something to fix it in Drupal 8 :)

One way to do this might be to merge them into a single "Edit menu" page that allows you to edit the menu and its links from the same screen. Then "List links" would be gone entirely. I wrote a contrib module for D7 called Simplified Menu Administration that does this.

(b) for other menus, you get a page that's titled "Edit menu link," which is not the same as "Edit menu." Suggest changing both tab and page title to "Edit menu item."

This doesn't occur anymore - I think it must have been a bug that was fixed a while ago? (The tab name is currently "Edit menu" in all cases.)

I also don't think we want to start using the word "menu item" on any of these screens. In Drupal, menu items and menu links are actually different things (you can have more than one link pointing to the same item, for example) and the Menu module allows you to create links, not router items. So I don't think it's a good idea to mix the terminology.

+1 on number 1. I tested out the Simplified Menu Administration module and it is much better than what is in core.

Marked #1164786: Users Expected “edit menu” to edit menu items. as a duplicate.

This issue was encountered during usability testing at the University of Minnesota, so I'm transferring the appropriate tags over from that issue to this one.

subscribing

I'd be OK with merging "List links" operations into the "Edit menu" page, as suggested by http://drupal.org/project/simplified_menu_admin

Although technically very different operations, that's a typical case of exposing too much technical abstraction in the user interface for no good reason.

However, speaking of reasons and potential consequences, this might lead to the problem of executing two (technically) conflicting operations at the same time. Currently not possible (yet?), but simply consider "renaming" a menu and changing links at the same time (might not be an issue due to machine names though...); or creating a new menu and immediately wanting to add links to it in the very same UI operation (would sound sensible to me). Stuff like that we've to watch out for.

+1. I've tested and it work like a charmed, after 20 minutes still can't find use case that give error in D7.

I've tried this also:
Edit description / Reordered menu links at once
Edit description / Hide row weights / Change row weight at once
Edit description / Show row weights / Reordered menu links at once

John

@David I like this approach, can you make a patch?

Here is a patch that merges the two pages. Very similar to what simplified_menu_admin module is doing.

Revised patch with fixes for failing test cases, which were largely due to a change in the name of the submit button, as well as a change in the breadcrumb structure resulting from combining the two forms. Test cases updated to reflect these changes.

Thanks mrfelton! This still trips me up at times. I know that besides Minnesota testing, this has also been observed in Gardens as an issue.

This needs before/after screenshots for review, if somebody could try the patch and report back with some, that would be great.

I really actually mocked up an idea for this yesterday, not aware of this issue. I'm not showing before we have screenshots :)

Screenshots attached.

These are the screenshots of mrfelton

I've tested it and tried to break things with no success. Looks really solid. I've spotted a little error in the patch:
@see theme_menu_edt_menu() => @see theme_menu_edit_menu()

Note: I had to clear the cache to have a working "edit menu" link.

Did a quick review of this patch - looks great! It may need a reroll though.

Couple comments:

1. +  $form['description_toggle'] = array(
   +    '#type' => 'checkbox',
   +    '#title' => t('Edit description'),
   +  );
   +  if (isset($form['description']['#access'])) {
   +    $form['description_toggle']['#access'] = $form['description']['#access'];
   +  }
   

   Seems like we could remove that #access manipulation if we just put the description and its toggle one level down (i.e., something like $form['description']['description'] and $form['description']['description_toggle']).

2. There is help text still referring to "list links" (I know this because I filed #1148342: Menu module help text still refers to "list links" when this module is enabled against the contrib module a while ago.)
3. Should we modify the Shortcuts UI in this patch also (like the module does)? To have a consistent experience we have to do that too eventually...

rerolled after core directory move.

Hm. That form constructor and submission handler is very complex. I'd suggest to retain the current form constructor, submit, and theme functions, so that contributed modules are able to use them for different user interfaces.

Attached patch retains the current functions.

Removed needless keys from menu_menu() definition.

To be consistent, we should do #1340500: Merge "list terms" page into "edit vocabulary" page too. Is there anywhere else this happens that we should also clean up?

Hm the Content Type and Field creation needs a lot of cleaning up in the workflow, but that is different and is completely baffling for new users instead of being just misleading once :P

Thanks for the cross-link jenlampton. I had a quick look through core and didn't find any other uses of this exact pattern. Forum comes close with the container/forum approach but naming and managing are not split out like with menu/links and vocabulary/terms so I think those are the only two.

Well, there's Shortcut, but as mentioned above that should probably be part of this issue.

The two UIs are very close to identical, so it really wouldn't be a good idea to have separate issues and end up with one of them changed but the other left behind.

Here's a re-roll of sun's patch, but also updated the menu tests this time.

I'm not sure I like how the checkbox for "Edit description" works.

1) when creating a new menu, there's no reason for the Description field to be hidden. There's actually no reason for the Edit description checkbox on the menu creation page at all.

2) when editing an existing menu, couldn't we just have used a collapsed fieldset instead? Adding an additional form element just to control the display of another one seems silly.

Yhea, I am not convinced its a very good pattern either. Lets try and brainstorm that part more.

I am going to revert it to needs review, because apart from that small design issue and bit of code - the rest of the code can be reviewed.

I was thinking we could maybe apply the machine name pattern here (http://drupal.org/node/1227546)

Edit: gah, forgot to update the text in that second textfield, should read "Contains all the links related to a user account." of course

That checkbox pattern isn't a new one (it's also used for the "Provide a menu link" checkbox on the content creation page, for example). And I think the idea of hiding the description even on menu creation was to reduce clutter. A lot of people probably don't bother entering menu descriptions at all.

That said, the mockup in #29 which suggests switching to the machine name pattern looks very nice.

Can we move the UI design exploration for the Description field into a separate issue? I think this patch/change makes sense on its own, and will even work if we don't manage to introduce a new fancy UI pattern for one-off optional form fields ;)

#26: menu_edit_links-663946-26.patch queued for re-testing.

Here is a straight reroll, since this did not apply anymore in many ways. I did not remove the checkbox => edit description thing, although I do agree that it might be easier to get this accepted as a start without that new pattern, so will look for removing that in a followup. Also might fail some new tests. I did find a user test that would have failed, so fixed that too (see interdiff).

Editing a menu I've just created (has no items but title is editable):

Editing a built-in menu (has items but title is not editable, neither machine name):

Remove the Edit description checkbox for easier acceptance as per @sun above. It is just less code for now (although I agree that it visually looks worse, there are no non-standard elements).

Could be wrong, but I interpreted @sun's comment as being about #29, not about the actual code in the patch.

The checkbox used in the patch is an existing pattern already used elsewhere in core (as mentioned above) ... though there are subtle differences in terms of when it has the "open" state.

I see that people had some issues with it here, and yes, we could remove it for that reason, but the problem with doing nothing is that the important part of the form is really pushed far down the page.

All right, let's take #34 as a starting point then and ignore #35 :)

I do not think we should introduce (or advance on) the checkbox-to-unhide-elements in this issue and patch.

Normally, this UI pattern ins applied to "optional advanced features", which are disabled by default, but can be enabled, and if they get enabled, you need to supply additional values. That doesn't really make sense in this situation, because the menu description is totally not an optional advanced feature of a menu — it's a basic property of all menus. If you leave it out, then the menu overview/listing page looks incomplete, since most other menus do have descriptions.

I agree with the UI problem of the link table being shifted down though. For now, I'd recommend to simply change the #type of the description from a 'textarea' into a 'textfield'. That way, the description field consumes much less space, and alas, the input widget would actually map much better to how the description is used elsewhere — a textarea suggests that you'd be able to use linebreaks and perhaps even HTML for the menu description; but the description should actually just be a short and simple one line summary.

If this suggestion doesn't fly either with you guys, then we should defer the discussion to a separate issue and just retain the current description form element without changes.

That said, this patch conflicts with #1814916: Convert menus into entities, which is quasi-RTBC, so we might want to wait for that to land.

All right #1814916: Convert menus into entities is in fact RTBC, so let's postpone this one. That makes menus config entities, so the menu editing form is with the config entity. I agree a one-line input for description would suffice.

The next things to fix after menu is entities:
#1882218: Remove static menu link creation for menus in menu_enable() and elsewhere
#1882552: Deprecate menu_list_system_menus() and menu_ui_get_menus()

Also menu links #916388: Convert menu links into entities

Suppose menu itself is just a bundle for menu link and block to be placed in layout

@andypost: #41 seems unrelated here - I guess your comment was meant for another issue.

#1814916: Convert menus into entities landed, so we can get back to business here. :)

Here is an almost straight reroll. I also changed the description to be a one line textfield not a textarea. This will most probably not pass because although it visually looks good, the link reorder and enabled checkboxes (essentially the list links portion of the form) submission does not seem to take yet. I wanted to post interim work because I'll need to switch gears for a while today.

Reviews welcome anyway on the intertwining mix of the OO entity code and procedural link listing code :)

The breadcrumb test was failing since it was testing for a menu label that was not present anymore (due to integrated editing page). The rest of the fails were due to the ordering and enabled checkbox not taking the submit handler. Mirrored the submit handler invocation like the form builder integrates the form items. This makes the menu changes go through properly (the submission handler was not invoked otherwise).

The "add" button should always be above the table, not above the forms.

@Bojhan: yeah, well, the current nature of the action links in Drupal is they are defined via hook_menu() and pushed in on all pages to a standard place at the top of the page. They are not freely positioned among other page elements as it stands now.

The action link is a problem, I agree.

Meanwhile, I'm no longer sure whether this change proposal is architecturally correct/good. Now that menus are entities, submitting this form will trigger various events to inform modules that the menu has been changed, even if only contained links were changed.

I wonder whether the actual problem is that users expect to find the links where the UI currently says "Edit menu"...?

In other words, could we resolve this by

1. simply flipping the meaning of "Edit menu" (== former "list links")
2. introducing a new, secondary local task "Edit ???" (== current "edit menu")

That is, because editing an existing menu certainly is a <10% operation. Menus aren't changed often, once they've been created.

The primary operation for existing menus is almost always to edit the contained links. But yet, we're putting the operation to edit the menu as the primary operation and landing page into user's faces.

What do you think?

I totally agree with sun. The problem is that users don't want to think about what they're clicking, so to update menu links they want to click the primary action for a menu. I believe this would be solved with the following labels, which are somewhat flexible, but specifically should in this order.

1. Edit menu (or Edit menu links)
2. Settings (or Menu settings)

I agree with the UI problem of the link table being shifted down though. For now, I'd recommend to simply change the #type of the description from a 'textarea' into a 'textfield'.

That makes sense to me, but I wonder what the upgrade path for that will look like...

In other words, could we resolve this by

1. simply flipping the meaning of "Edit menu" (== former "list links")
2. introducing a new, secondary local task "Edit ???" (== current "edit menu")

This makes sense to me too, if we can only figure out what to replace the ??? with :) I think it's going to be hard to come up with a word that's really clear to everyone, and I believe that was part of the motivation for the original design; users think of all these things as part of the same "menu".

Meanwhile, I'm no longer sure whether this change proposal is architecturally correct/good. Now that menus are entities, submitting this form will trigger various events to inform modules that the menu has been changed, even if only contained links were changed.

I'm not sure it's actually wrong to say that a menu has changed when the links within it have changed...? Kind of depends on your point of view. In any case, if a module does not want to act every time a menu is saved (but rather only when property X changes during the save) it will have to deal with that either way, since there's no guarantee that property X changed during a "real" save either.

Now that menus are entities, submitting this form will trigger various events to inform modules that the menu has been changed, even if only contained links were changed.

If I'm not mistaken it should be quite easy to quickly check 'title', 'machine_name' and 'description' whether they have changed and, in case nothing has, not submit/save the menu entity at all. So we really should be having the merge-or-don't-merge discussion separately from the technical aspects I think.

That said, I personally think "Edit menu" and "Edit ???" will be almost certainly confusing regardless of what ??? is, and I don't really get why we bailed on the merging of the two pages so quickly.

I think we could even hack around the local actions issue with a quick hook_page_alter(). That would obviously be ugly as hell, but if the placement of the "Add link" link is the only thing really holding this up...

I agree with tstoeckler and David. I think the user perceives the menu has a name, description and has links in it. Consider if you would need to go edit the free tagging terms associated to a node at a separate UI just because they are separate entities. No. You have an easy all-in-one interface for them. Same goes for the menu. Yes, the items are technically other entities, but so are terms or file uploads on nodes. Eg. if you change the file description on a file uploaded to a node, that would not have any change on the node itself, only on the associated file, but we naturally do it as part of the node administration anyway (and yes, the node save hooks are fired).

I'm not fussed one way or the other, I like the idea or merging the two as much as I like renaming the "Edit menu" link, but...

Consider if you would need to go edit the free tagging terms associated to a node at a separate UI just because they are separate entities. No. You have an easy all-in-one interface for them.

I guess the same should apply to content types and field definitions. Why can't you edit the order of fields on the same page that you edit a content type definition, in the same way we're proposing to edit the order of menu items on the menu definition edit page?

I suspect this comment isn't really constructive but *could* be used as an argument against the current proposal.

Why can't you edit the order of fields on the same page that you edit a content type definition, in the same way we're proposing to edit the order of menu items on the menu definition edit page?

If a node type were just a title and a description, I would totally vote for doing that. As it stands now, there are about 100 other things that you can configure on the node type edit page, so that merging the two pages would be a huge UX regression.

Didn't mean to remove that tag...

Sorry guys, but shouldn't we postpone this on #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)? Let's convert to kewl ListControllers first and then add new stuff on top! :)

Also related issue #1798540: Add link to add a new entity in an empty entity list controller table

I don't see a particular reason to postpone this issue on that one, no. Whichever one gets to RTBC first wins. :)

Hmm, I don't think @sun is correct. Although our system might dictate the idea, of rebuilding the whole menu system upon "Editing the menu", to users there is conceptually only thinking of one editing that of the menu and its links. That the system creates a hard separation between the name/description and its actual items is one, of the system - and not one, that needs to disturb the mental model of the user. If our implementation is so inflexible we cannot do this, then that sounds like a problem. It is true, you might not need to edit it much - we will need perhaps to apply a pattern (e.g. collasped fieldset).

I dont see the necessity to take this to the extremes proposed by @mstrelan, the user mental model of building a content type and its fields is one that cannot be tackled by this UI improvement. The field UI needs a whole revamp, where indeed "edit" means being able to quickly switch between the fields and entity (node type) editing.

I think we will still need to move the action closer to the table, or we will regress from an consistently applied pattern. I think in all this is about bringing similar operations closer, and not creating hard distinctions between actions that to the user are in the same mental space. This is difficult for Drupal, but I imagined with all our new abilities we where able to do it.

I'd like to suggest a different pattern - the menu case is not different from other administrative screens (vocabularies, shortcuts)
So we just need a listing form to be embeddable/inlined in to other's entity form!

[Actions] - #1839516: Introduce entity operation providers
[Entity edit] - entity edit form
[Sub-items sortable list] - entity list controller's render()

Half of this listings are implemented as sortable forms already and cleaned-up in #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)

Also it would be good to have a simple form to edit title and machine name within listing page by some ajax magic. a-la LDO does for translations.

So no matter the order

@fubhy/@andypost: re postponing on #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController) (and other work on entity listings such as #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController) proposed by @andypost) remember menu items are neither config nor content entities (yet), so postponing on adding features to entities would also postpone making them entities. That seems pretty far off at this point (to me).

@Bojhan: we can manually add similar markup to the middle of the form easily and unmark the link as an action link and make it a regular menu callback. That would very easily give us the desired visual presentation. Not a very standard implementation. I think probably trying to make out some solution to place action links "more officially" at non-standard places might wind up being hacky either way :D

#45: menu_edit_links-663946-45.patch queued for re-testing.

Any other concerns? I'd love to work on just "moving" the action link (ie. unmarking it as an action link in hook_menu and putting in equivalent markup in the middle of the form I guess), if that seems to be the only immediate problem we should resolve before commit. :)

First a reroll of #45 as-is, so it still cleanly applies.

Implemented moving the action link as per Bojhan's request. I don't think we have a better way. Grepping through Drupal, the ul container for action links is in the theme (page.tpl.php), so not really possible to produce with a theme callback. The item itself has a theme callback that we reuse and parameterize proper. Reviews please :)

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -21,6 +21,7 @@ class MenuFormController extends EntityFormController {
+    $form_state['menu'] = &$menu;

@@ -46,10 +47,23 @@ public function form(array $form, array &$form_state, EntityInterface $menu) {
+      $form['links'] = array();
+      $form['links'] = menu_overview_form($form['links'], $form_state);

+++ b/core/modules/menu/menu.admin.inc
@@ -47,20 +48,35 @@ function menu_menu_edit(Menu $menu) {
-function menu_overview_form($form, &$form_state, $menu) {
+function menu_overview_form($form, &$form_state) {
...
+  $result = db_query($sql, array(':menu' => $form_state['menu']->id()), array('fetch' => PDO::FETCH_ASSOC));

@@ -74,20 +90,23 @@ function menu_overview_form($form, &$form_state, $menu) {
+      'href' => 'admin/structure/menu/manage/' . $form_state['menu']->id() . '/add',
...
+  $form['#empty_text'] = t('There are no menu links yet. <a href="@link">Add link</a>.', array('@link' => url('admin/structure/menu/manage/' . $form_state['menu']->id() .'/add')));

FormController already stores entity within. So why not pass the entity as parameter? and do not change the parameters for form?
This change makes the form builder dependent on internal form_state structure!?

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.php
@@ -46,10 +47,23 @@ public function form(array $form, array &$form_state, EntityInterface $menu) {
-      '#type' => 'textarea',
+      '#type' => 'textfield',

This simplifies a screen but what's to do with custom Menus which could be more then one line? EDIT - the upgrade tests would fail

@andypost in fact it would be simpler patch to not pass the menu around with form_state, the form_state based solution was introduced by @sun above on November 12, 2011 (comment #19) in the name of letting form altering work easier. There is an old pattern where the main object for the form was somewhere on the top level of the form structure, but a more correct approach is to use form_state.

As for how the "the upgrade tests would fail" for changing the menu description from a textarea to textfield I don't get. For fact, the tests actually don't fail (see above), and the textfield itself does not limit the size of the value significantly. Where are menu descriptions displayed outside of the admin page for menus? Reality is core menu descriptions are pretty short so I don't see what we'd be loosing by making the UI for it reflect our intended use of it.

Any other concerns?

#66: menu_edit_links-663946-66.patch queued for re-testing.

I've read the entire issue. AFAICT, all concerns were addressed. Applied cleanly, manual testing works fine. Almost RTBC.

I read the patch, and found a bunch of nitpicks.

+++ b/core/modules/menu/lib/Drupal/menu/MenuFormController.phpundefined
@@ -46,10 +47,23 @@ public function form(array $form, array &$form_state, EntityInterface $menu) {
+      // Form API supports to construct and validate self-contained sections

s/supports to construct and validate/supports constructing and validating/

+++ b/core/modules/menu/menu.admin.incundefined
@@ -74,20 +90,23 @@ function menu_overview_form($form, &$form_state, $menu) {
+  // Inline the add link action so it displays right above the table of links.

s/Inline the add link action/Inline the "Add link" action/

+++ b/core/modules/menu/menu.admin.incundefined
@@ -183,15 +202,21 @@ function _menu_overview_tree_form($tree, $delta = 50) {
+  $parents = $form_state['menu_overview_form_parents'];
+  $input = NestedArray::getValue($form_state['input'], $parents);
+  $form = &NestedArray::getValue($complete_form, $parents);

These 3 lines are what make the long comment in MenuFormController.php reality. Very nifty. I think we should repeat the essence of that comment here, and add an @see.

+++ b/core/modules/menu/menu.admin.incundefined
@@ -183,15 +202,21 @@ function _menu_overview_tree_form($tree, $delta = 50) {
+  // Get the $_POST order.

s/$_POST/POST/, since we never are allowed to use $_POST anymore?

@Wim, thanks resolved all of those!

Functionally identical; only comment changes in #71. Should come back green.

I think the issue with the upgrade path is the same one I was referring to in #51.

More specifically:

1. Existing sites with newlines in their menu descriptions will have them disappear when loaded into the textfield (not so terrible actually, but it does cause sentences to run together - I think we need to convert the newlines to a space on upgrade).
2. Bigger issue: Any existing menu description longer than 128 characters cannot be re-saved, since the textfield has a maxlength of 128.

All right, I think the overall change is a lot more important than debating this single field, so I moved it back to be a textarea. We'll have the opportunity to debate this for much longer in other issues.

     $form['description'] = array(
-      '#type' => 'textarea',
+      '#type' => 'textarea',
       '#title' => t('Description'),
       '#default_value' => $menu->description,
     );

Can't figure that one out since the lines look identical...

In any case, why not just set the #rows property to 1 to at least make it take up as little vertical space as possible (it won't even look too different from a textfield at that point)?

I see you want me to work more on this simple update. There is nothing wrong with a diff removing a line and then adding it back in the same way (and it was the quickest way to update the patch to resolve your concerns). However, here is a full new patch with an interdiff for extra fluff.

I don't think making the textarea appear one line would solve your concerns for multiline strings either, since they would not appear to show up. As said above, we can resolve that in followups, changing the field was not an objective of this issue, the objective is to merge the pages.

WOOHOO!

No more nasty "list links" link:

Now both the menu name/description and menu links on the same page, "Add link" button at the top of the table, "Description" is a textarea to address David's concerns:

I believe that's everything. Seems to work great. :)

Committed and pushed to 8.x. YEAH!!! So nice to have this silly UX issue behind us.

Ahem.

Tracking changes that need clean-up.

Honestly, I do not understand what we did here.

@Gábor just posted this screenshot in #1889150-32: Simplify overlay look, *only use for contextual operations*:

Problem

1. For built-in menus, the first form control you see is a disabled text input field that you cannot edit, and which is completely irrelevant for the task you're performing.
2. The second input field is a description that is not output anywhere, except for the administrative listing page; i.e., completely irrelevant for the task you're performing in >95% of all cases.
3. For custom menus, the first form element is the menu's title, but if you click the "Edit menu" link on a menu block and you change that title and save the block, then you have changed the menu's title, but NOT the block's title. In turn, the form element is not only irrelevant, but also misleading and confusing for users, since it doesn't do what it is supposed to do.
4. As a result, the user is confronted with primary user interface controls that he's almost guaranteed to be not interested in.

I still do not understand why we did this instead of #49; i.e., just re-purposing/re-labeling the tabs/links.

@sun I think the next step is to make this form, not so super in-the-way. However I disagree that this would be a regression, which is what you are implying - now they are not confronted with clicking "edit" and not finding the actual links. Now at least they find the links, your solution was still one where there are two places. In any concept, we would pursue it would not be two places

@sun: The problems you are pointing to are pre-existing and they are just more in the face now that menu editing is one action and this menu edit screen that all inexperienced people went at first earlier is now here although you have not seen it before because you knew not to click on there :)

- I'd argue if there is any use for a disabled text field displaying an internal value that is not useable for anything for the user. We had the same issue for block machine names displaying back in a non-editable form but the UX footprint of that was greatly decreased by making it a standard machine name control, so it takes up much less space.
- On the description, see above on people freaking out to even make it a one line field, because it would break existing data. I've asked where it is used elsewhere outside of the menu admin for example :)
- On editing the menu title not editing the block title and vice versa is a pre-existing problem. In Drupal 7 we had this awkward setup where if you left the block title "empty", then it inherited from the menu, if you put in "" then it went empty, and otherwise it was also disconnected from the menu. In Drupal 8 my understanding is the block gets the title copied on creating the block and there is no inheritance capability anymore. One could argue that moving into this direction with blocks displaying an object with a title is still not intuitive, but in classic Drupal fashion, it is a confusing flexibility, where it lets you reuse the same menu with different block titles for example. (Theoretically?)

@Gábor Hojtsy

It seems to me that this might be solved through language. If blocks are really just shells then should we even reference them in the field label? What if the field labels were:

Menu name (For administrative use)
Main menu

Menu title (Displayed to the user)
Banannaphone

I'm not sure, but I think this pattern extends:

Custom block name (For administrative use)
Twitter feed

Custom block title (Displayed to the user)
Tweets

etc.

Where the block already has a name which is also the title we would need to show the name on the form so the user sees the clear distinction, for instance:

Form name (For administrative use)
Search form

Form title (Displayed to the user)
Search this site

As in the above example, where the name is specific and not generic we also need a generic field label. These would generally inherit the name of the module producing the block so: View name, Form name, Menu name, Content name, Custom block name, and for things like "powered by Drupal" maybe "System block name".

This would impose some consistency across the blocks system and module developers can decide if they want to default a name into the field. It would also remove the *very* problematic situation that currently exists in blocks where:

• I go to add blocks and choose click "configure" on the recent comments block.
• I change the title from recent comments to "Recent not comments" and click save.
• Back on the blocks UI I still see "recent comments" (?!)
• But when I click configure, my configuration page says "Recent not comments"!

@tkoleary: Drupal is too flexible that we know what kind of data is used where. The menu name for a custom created menu will be copied to the block title, however later on the block title is disconnected. The menu description is only currently displayed in the admin UI *in core*, so we can probably safely say it is an "administrative description" like in views:

Also, this is a closed issue, the goal of the issue was fulfilled. Whatever we keep discussing here will just be lost and not visible for further work, so better stop discussing unrelated things here and move on to other issues AFAIS.

so better stop discussing unrelated things here and move on

Wow. Unrelated? It's great to see major concerns brushed away like this.

None of the problems stated in #80 actually have been addressed. The committed changes made the UI/UX considerably worse. There's also no path forward at all, which would fix the mess.

Seriously, I wonder whether you guys just coded this down and reviewed the interface/experience solely based on screenshots. If you actually try to use this, then you'd immediately recognize that the new interface is totally confusing and something is very very wrong.

Worst of all, the change was needlessly implemented in a way that makes it very hard to untangle and decouple the two forms again, so a contributed/custom module has to perform quite some advanced leg work to undo this weird interface change.

The amount of code debt and one-off hacks that Spark is introducing very recently is getting very frustrating. If all core initiatives and efforts would work like that, HEAD would be a big pile of total mess.

@sun: none of the form elements have changed in this patch. The problems you pointed out with the description, disabled form fields, etc. already existed => which is why I'm saying they are unrelated.

Worst of all, the change was needlessly implemented in a way that makes it very hard to untangle and decouple the two forms again, so a contributed/custom module has to perform quite some advanced leg work to undo this weird interface change.

The amount of code debt and one-off hacks that Spark is introducing very recently is getting very frustrating. If all core initiatives and efforts would work like that, HEAD would be a big pile of total mess.

As for the one-off hacks that are "introduced by Spark", the form setup that makes "very hard to untangle and decouple the two forms again" was suggested/introduced by *you* in comment #19 above with the following notes:

Hm. That form constructor and submission handler is very complex. I'd suggest to retain the current form constructor, submit, and theme functions, so that contributed modules are able to use them for different user interfaces.

Looks like you don't agree with yourself anymore?

Looks like this should still be open for followups, etc. I think a summary of the current situation is that this patch made things better for new users (who would never have even found the page before), but worse once you already know where the page is.

Of the issues listed above, this one is particularly bad:

For custom menus, the first form element is the menu's title, but if you click the "Edit menu" link on a menu block and you change that title and save the block, then you have changed the menu's title, but NOT the block's title.

I think it's probably a pre-existing problem in Drupal 8 (not introduced here), but a pretty major one. In Drupal 7 this would only happen if you deliberately configured the block to have a different title than the menu to begin with. By default, though, it would work as expected. In Drupal 8, there doesn't seem to be any way to have the block title automatically linked to the menu title any more?

---

There is nothing wrong with a diff removing a line and then adding it back in the same way (and it was the quickest way to update the patch to resolve your concerns).

Right, I was just trying to figure out how in the world a patch file like that would even be generated - i.e., I wondered if there was an extra invisible character being introduced. I think the answer is that you do it by editing the patch file by hand :)

We still need to figure out what to do about the enormous description field that now dominates the page. That part is definitely a regression introduced here... If #rows=1 is too small, then maybe #rows=2 would be big enough to still see there's a scrollbar there when necessary? (It probably depends on the browser.)

@David:

1. That object titles and objects displayed in blocks have disconnected titles is a behavior of the Drupal 8 block system yes. For example if you go create a custom block, you can give it an "administrative description" that is used to initially populate the block title. So if you accept the defaults they are the same text. (Just like with menu blocks). Then if you hit the "edit" contextual menu on the block, you can edit the underlying content entity, but not the block title. If you hit "configure block" then you can edit the block title but not the underlying content entity. Both have titles, and they are the same in the default setup. The same holds true for menu blocks, aggregator blocks, language module blocks, etc, etc. I don't think this is related to this issue. I'm also not sure if this is merely by design in the current system. Opened #1926736: Allow block titles to be automatically tied to the underlying object for it nonetheless and added to #1875252: [META] Make the block plugin UI shippable for tracking for blocks UX.

2. I opened a followup for that disabled machine name display that @sun also called out: #1926676: Useless editing widget for built-in menu labels

3. On the description, @sun's comments above also hold lots of truth. The description is only ever used on the menu listing admin page, and there is no reason to let people enter a novel here. Did not find an existing issue for this, so submitted: #1926692: Menu description is misleading to allow for multiline input

4. I've also opened a followup for the action links workaround that we used above: #1926732: Figure out a better way to inline action buttons - ideas welcome :)

Any other issues to open/track?

That's all the ones I can think of, thanks.

Another issue #1938918: Convert menu theme tables to table #type that affected by the lost sanity here but could improve state and Usability

+++ b/core/modules/menu/lib/Drupal/menu/MenuListController.phpundefined
@@ -47,13 +47,8 @@ public function getOperations(EntityInterface $entity) {
-    $operations['list'] = array(
-      'title' => t('list links'),
-      'href' => $uri['path'],
-      'options' => $uri['options'],
-      'weight' => 0,
-    );
     $operations['edit']['title'] = t('edit menu');
+    $operatuins['edit']['href'] = $uri['path'];
     $operations['add'] = array(

typo

+ $operatuins['edit']['href'] = $uri['path'];

#2019735: operatuins typo in MenuListController.php

added a related and followup section while I was adding my follow-up. -y