The pattren used at http://drupal.org/node/302054 could be reused to tackle the issue on Menus in which the first item a user can fill out is not the actual title but the machine readable name.

Menu name: (machine readable format)
Title:
Description:

Changing this to just Title. We probably have to edit the previous patch a bit, since it seems non-reusable js.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Bojhan’s picture

kika’s picture

Proposed machine name description:

"The unique machine readable name for this menu, can only contain lowercase letters, numbers and hyphens."

jix_’s picture

Mockups!

Note also that, for consistency, I called the fields "name" and "machine name" instead of "title" and "machine name".

clemens.tolboom’s picture

When editing the menu only the description is available. Why not the Title?

#273137: Split Navigation to User and Administration menu

clemens.tolboom’s picture

FileSize
3.14 KB

Patch contains a more generic solution for 'machine readable' by providing a misc/auto_machine_readable.js which is a generic variant of the content_type solution.

When committed we could change content_type the same way.

clemens.tolboom’s picture

There is some similarity with #503816: Make form elements expandable concerning open/display behaviour.

clemens.tolboom’s picture

Status: Active » Needs review
kika’s picture

Issue tags: +d7uxsprint
catch’s picture

vocabulary.js also uses this pattern for machine readable names.

Clemens - how will your js account for the fact that content types and vocabularies only allow underscores and menu only allows hyphens?

kika’s picture

clemens.tolboom’s picture

@catch: good point :)

Guess there is a design pattern arising for a 'linked-field' widget with a linked-field function to execute it's behaviour.

There is also a linked label in menu.js

Drupal.checkPlain($('#edit-menu-link-title', context).val()) || Drupal.t('Not in menu');

replace to hyphen for machine readable menu
replace underscore for vocabulary
replace as is for menu title/fieldset on node/add and node/edit

seutje’s picture

seems related to #410464: Menu name = node title upon node edit through user interface

maybe try to find 1 solution for all cases?

Dries’s picture

When I looked at the screenshot, I couldn't remember what the machine name was for. I had to read the description in the old UI to figure it out. With the new proposed UI that 'critical' explanation is missing. I wonder if we should call this 'URL' or 'URL path' instead of 'machine name'?

clemens.tolboom’s picture

Old text is

"Machine Readable name"

and

"The machine-readable name of this menu. This text will be used for constructing the URL of the menu overview page for this menu. This name must contain only lowercase letters, numbers, and hyphens, and must be unique."

According to http://nl2.php.net/parse_url it is called "path"

Proposal:

URL Path

and

"This text will be used for constructing the URL for this menu. This name must contain only lowercase letters, numbers, and hyphens, and must be unique."

attiks’s picture

I like the wording in #15, it's much clearer for the end-user

clemens.tolboom’s picture

Assigned: Bojhan » clemens.tolboom
clemens.tolboom’s picture

FileSize
3.37 KB

Changed the text almost accordingly ... "for constructing" => "to construct" and fixed underscore replacements by hyphen.

kika’s picture

Tested, works as expected to me, nice standardization. +1

Side note: we might to have a separate issue about enabing editing existing url paths for menus, using the same UI.

Dries’s picture

Status: Needs review » Needs work

First quick review:

- "URL Path" should be "URL path"

- In file names we should use dashes when we can.

- There are some spacing/indentation/tabs issues in the Javascript.

A more in-depth review is still required.

clemens.tolboom’s picture

Category: task » feature
clemens.tolboom’s picture

FileSize
3.33 KB

#20 issues applied.

Bojhan’s picture

Status: Needs work » Needs review

UX wise thumbs up.

moshe weitzman’s picture

Status: Needs review » Needs work
+    drupal_add_js( 'misc/auto_machine_readable.js');
+    drupal_add_js(array( 'autoMachineReadable' => array('title' => 'menu-name')), 'setting');

should use #attached_js instead. grep for examples. there is a mention of this in the upgrade docs as well.

clemens.tolboom’s picture

Status: Needs work » Needs review
FileSize
4.47 KB

* Applied #24 suggestions

* Added documented to auto-machine-readable.js

* Made auto-machine-readable.js more general applicable.

* I added a check for the error class on "Menu Name" due to a usage problem with menu titles longer then 27 chars. The hidden field is called "Menu Name" but the UI is not showing this anywhere. Now the field is not hidden on errors

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

I am going to mark this RTBC, hasn't gotten a review in 7 days. And doesn't seem like any objections are made.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+//    drupal_add_js( 'misc/auto_machine_readable.js');
+//    drupal_add_js(array( 'autoMachineReadable' => array('title' => 'menu-name')), 'setting');
     $menu = array('menu_name' => '', 'title' => '', 'description' => '');

These need removing.

Could you open a followup issue for removing vocabulary.js and content_types.js in favour of automachinereadable?

clemens.tolboom’s picture

FileSize
4.19 KB

comments removed.

clemens.tolboom’s picture

clemens.tolboom’s picture

Hmmm there is a difference between menu add and taxonomy/content type.

Menu add has a _hidden field_ which comes visible only when the user clicks on the edit link.

Taxonomy and Content type both show the machine readable field _and_ haves an edit link.

xrefs:

#444402: Remove cruft from JavaScript code
#413192: Make taxonomy terms fieldable
#302054: Usability: Hide machine readable name of node type by default.
#471070: coding style fixes

We need to check #477020: Content type machine readable name field does not recieve focus

clemens.tolboom’s picture

FileSize
4.37 KB

- Fixed for/based on #477020: Content type machine readable name field does not recieve focus
- added js var prefix for variables.

[Edit]
- added text param for general usage

kika’s picture

1. I do not like auto-machine-readable construct, perhaps we should stick to something more descriptive like machine-readable-name / machine-readable-title.js / machine-readable-field.js etc?

2. I see a further abstraction in future follow-up buy creating a new form item type, say 'textfield_machine' what attaches js, passes custom properties to javascript and does rest of the work. So instead writing this:

$form['menu_name'] = array(
  '#type' => 'textfield',
  '#title' => t('Menu name'),
  '#attached_js' => array(
    'misc/auto-machine-readable.js',
     array(
       'data' => array(
       'autoMachineReadable' => array(
          'title' => array(
          'text' => 'URL path',
          'target' => 'menu-name',
          'searchPattern' => '[^a-z0-9]+',
          'replaceToken' => '-',
        ),
      )
   ),
   'type' => 'setting')
   ),
...
);

you could write

$form['menu_name'] = array(
  '#type' => 'textfield_machine',
  '#title' => t('Menu name'),
  '#machine_text' => 'URL path',
  '#machine_source' => 'title',
  '#machine_search_pattern' => '[^a-z0-9]+',
  '#machine_replace_token' => '-',
...
);
clemens.tolboom’s picture

The more fundamental construct is about calculating a field value from another fields value. The current construct is doing only replacements when the field is _not visible_ .

As a FAPI extension I don't like all those #machine_* attributes.

But you have a point for naming the .js ... auto-machine-readable.js

From all your options I would vote for a new one machine-readable-value.js or even machine-value.js

because it's just a calculated value. And we need it for taxonomy and content_types too.

Jeff Burnz’s picture

Subscribe

Bojhan’s picture

machine-readable-value sounds good to me, its a calculated thing so would be rather silly to keep to the user facing field or title logic.

Let's keep 2. of kika's comment for a follow up.

clemens.tolboom’s picture

FileSize
4.34 KB

- Renamed js to machine-readable-value.js
- adjusted menu.admin.inc accordingly
- Wrapped t() around 'URL path'

kika’s picture

I like it now. Go, Clemens!

Bojhan’s picture

Status: Needs work » Reviewed & tested by the community

Going to mark this RTBC, but sun will review.

sun’s picture

Status: Needs review » Needs work
+      '#description' => t('This text will be used to construct the URL for this menu. This name must contain only lowercase letters, numbers and hyphens, and must be unique.'),

Not sure whether it should be "lower-case". (I'm no native speaker)

"for this menu" => "for the menu" ?

+            )
+          ),
+          'type' => 'setting')

Since the bot did not fail, some weird indentation goes on here. Don't forget to add a trailing comma when moving that closing brace.

+      '#weight' => 0,    

(and elsewhere) Please remove all trailing white-space.

Additionally, I'm questioning why we need a #weight here at all. If you want to re-order the fields, then re-order them in code. If you cannot re-order in code for any reason, then at least make sure the 'submit' element gets a higher weight (10 or better 100).

+++ misc/machine-readable-value.js	1 Jan 1970 00:00:00 -0000

That should go into system.js, no? At least, all it does is to provide a behavior, and behaviors are provided by modules.

+ * @see menu.admin.inc function menu_edit_menu

Wrong syntax for @see.

+    var mrvs = Drupal.settings.machineReadableValue;
+    for(var mrv in mrvs) {
+      var searchPattern = mrvs[mrv].searchPattern;
+      // We do global search
+      var searchPattern = new RegExp(searchPattern, 'g');
+      var replaceToken = mrvs[mrv].replaceToken;
+      var replaceMultipleToken = new RegExp( replaceToken + '+', 'g');
+      var source = '#edit-' + mrv;
+      var suffix = source + '-suffix';
+      var target = '#edit-' + mrvs[mrv].target;
+      var wrapper = '.' + mrvs[mrv].target + '-wrapper';
+      var text = mrvs[mrv].text;
+      if (!$(target).hasClass('error') && ($(target).val() == $(source).val().toLowerCase().replace(searchPattern, replaceToken).replace(replaceMultipleToken, replaceToken) || $(target).val() == '')) {
+        $(wrapper).hide();
+        $(source).keyup(function () {
+          var machine = $(this).val().toLowerCase().replace(searchPattern, replaceToken).replace(replaceMultipleToken, replaceToken);
+          if (machine != '_' && machine != '') {
+            $(target).val(machine);
+            $(suffix).empty().append(' ' + text + ': ' + machine + ' [').append($('<a href="#">' + Drupal.t('Edit') + '</a>').click(function () {
+              $(wrapper).show();
+              $(target).focus();
+              $(suffix).hide();
+              $(source).unbind('keyup');
+              return false;
+            })).append(']');
+          }
+          else {
+            $(target).val(machine);
+            $(suffix).text('');
+          }
+        });
+        $(source).keyup();
+      }
+    }

Holy cow.

a) No abbreviations in variable names, please.

b) Comments should form proper sentences; missing period (full-stop).

c) Speaking of comments, there's quite some logic in here, which I don't understand at first sight. Only trivial code does not need comments.

clemens.tolboom’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for the review.

1. lowercase is taken from old code. I'm not a native speaker either :(

2. for this menu is taken from old code. Personally I like 'this' over 'the' menu. It's more task focused.

3. I'll check the weird indentation

4. The weight was indeed questionable the first day I touched the code. I will change that if possible. The code is used for both add and edit.

5. system.js ... I have to look at that js. But as in #29 mentioned this is more generic js code.

6. Holy cow. a. I will change that. b. I'll check it. c. I 'stole' the code from content_type which had no comments what so ever. I agree with 'more comments please'.

yoroy’s picture

Clemens, any chance you'll revisit this patch before code freeze?
I'm noticing in current HEAD that the machine name is still above the human title, would like to know if I should create a seperate issue for that or if we will fix it here. Thanks!

stBorchert’s picture

Status: Needs work » Needs review
FileSize
5.23 KB

Bojhan asked me to re-roll this patch.
@sun: the fields can't be reordered in code, so I've user '#weight' => 10, for the submit button.

Added some comments to the js function (as far as I understood the code).

dmitrig01’s picture

This patch converts node types to use the same mechanism

Bojhan’s picture

I hope tha_sun can do a final review, and then this should totally go in!

catch’s picture

Title: Hide machine readable name of menu by default. » Generic pattern/js for hiding machine readable names (applied to menu and content types)
Category: feature » task
Priority: Normal » Critical
FileSize
13.56 KB
13 KB

This looks good to me, I prefer line breaks before inline comments when the code is dense like that, but might be just me.

Just a note that we also have vocabulary.js now for vocabulary human readable names - this can be converted once this one's in and it's a direct copy of the content-type.js anyway, so should be a cut and paste conversion.

I'm bumping this to critical, since it's going to result in 1/3rd less javascript for 1/3rd more machine-readable name hiding - and it ought to be really useful for contrib modules with the same need. it also came up in Baltimore (although only one participant) - http://drupalusability.org/node/69

There's no visual change for content types. Here's before/after for menus:


stella’s picture

This looks awesome!! I tested the changes for both the menu and content type changes in FF3.5 and also IE8 and had no problems.

+1 to get this in

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Oke, tha_sun reviewed, dmitrig corrected and reviewed, and stella. I dont know what more we can do, lets get this in!

sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.19 KB

There you go.

catch’s picture

Status: Needs review » Reviewed & tested by the community

sun's comment changes in the js file look good. Back to RTBC.

sun’s picture

Actually a bit more than comments.

- Revamped the form builder function to remove #weight properties.

- Slightly reworked the JS behavior to be much more readable.

I verified that the functionality still works. Another test by someone else wouldn't hurt though.

catch’s picture

Re-tested menus and content types, it does indeed still work.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

sign’s picture

Rerolled patch #49, looks like webchick commited some code to content_types.js on 21st Aug.

Attached images, all seems to work ok

sun’s picture

Status: Needs review » Reviewed & tested by the community

Ready to fly.

Bojhan’s picture

Looking good

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Couple quick observations that might or might not need to be addressed:

+++ modules/menu/menu.admin.inc	22 Aug 2009 16:39:13 -0000
@@ -402,43 +402,54 @@ function menu_edit_item_submit($form, &$
+  $form['#title'] = $menu['title'];

It looks like this needs to be run through check_something() since it's output to the page.

+++ modules/node/content_types.inc	22 Aug 2009 16:39:13 -0000
@@ -78,10 +77,23 @@ function node_type_form(&$form_state, $t
-    '#field_suffix' => ' <small id="node-type-name-suffix"> </small>',
+    '#field_suffix' => ' <small id="edit-name-suffix">' . ($type->locked ? t('Machine name: @name', array('@name' => $type->type)) : ' ') . '</small>',

I think there is still value in showing the machine-readable name even if it is not editable (for themeing, etc.). Unless I'm reading this wrong.

Beer-o-mania starts in 9 days! Don't drink and patch.

sun’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
11.17 KB

- #title is gone. I have absolutely no idea who introduced that and why, but it wasn't used anywhere and it doesn't make sense at all.

- The machine readable name is output in a non-alterable way (i.e. when $type->locked == TRUE) when the content-type is locked, otherwise, it's alterable with an [edit] link, so it's actually the way you think it should be. :)

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Great! Committed to HEAD. :)

Please make note of this new convention and how to implement it in the module upgrade guide.

sun’s picture

Component: menu system » javascript

Strange category.

sun’s picture

Status: Needs work » Needs review
FileSize
3.24 KB

well, as promised - that's it.

If you look closely at the removed vocabulary.js, you'll start to question how that could have been committed in the first place. But luckily, we'll remove it now.

sun’s picture

Tests passed. So this just needs a human test confirming that everything works as it should.

The latest patch only changes the taxonomy vocabulary add/edit form to re-use the new, generic machine readable name behavior - so that's all that needs testing.

sign’s picture

Tested, works well, re-attaching screenshots

Only one note/concern:
1) editing a Name of an already existing content type, without previously changed machine name, the machine name is being updated too (03_menu_after_title_entered.png)

2) when editing Name of a content type with custom Machine name, the edit js is not present (04_menu_after_title_entered_edit.png)

My main concern is, that some modules have settings on a separate screen, rather than content type edit screen (organic groups I think have that).
Can't users easily break their site up?
Shall we warn developers that this can happen?
Shall we warn users when they are changing content type name?
Shall we not change content type machine name when content type is being edited (I mean automatic js change here)?
Or do we have/will we have a hook or a method how to check if content type settings have been updated for developers?

sun’s picture

Please test the vocabulary add/edit page only. The latest patch only affects that page.

The severity of a machine readable name change depends on the system object that is being edited. Normally, modules should be intelligent enough to adjust all stored data and references for the new name. If something is not that smart, it should prevent editing the machine readable name.

sign’s picture

Tested on taxonomy add/edit vocabulary, works as expected

attached screenshots

sun’s picture

Status: Needs review » Reviewed & tested by the community

I can only guess that you forgot to change the status to RTBC then. ;)

sign’s picture

ooops, yes, sorry for that. RTBC! :)

pwolanin’s picture

Issue tags: +API change

tagging

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay removing crufty code!

Committed to HEAD.

Status: Fixed » Closed (fixed)

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

Chris Gillis’s picture

Currently it seems the auto-machine-readable feature is implemented on content type names by default.
Shouldn't this be applied to field names by default?