Posted by Bojhan on May 23, 2009 at 5:31pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | javascript |
| Category: | task |
| Priority: | critical |
| Assigned: | sun |
| Status: | closed (fixed) |
| Issue tags: | API change, d7uxsprint, Needs Documentation, Usability |
Issue Summary
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.
Comments
#1
Would require http://drupal.org/node/471242
#2
#302054: Usability: Hide machine readable name of node type by default.
#471242: Abstract out [edit] interaction on add content type
Check for #477020: Content type machine readable name field does not recieve focus too
#3
Proposed machine name description:
"The unique machine readable name for this menu, can only contain lowercase letters, numbers and hyphens."
#4
Mockups!
Note also that, for consistency, I called the fields "name" and "machine name" instead of "title" and "machine name".
#5
When editing the menu only the description is available. Why not the Title?
#273137: Split Navigation to User and Administration menu
#6
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.
#7
There is some similarity with #503816: Make form elements expandable concerning open/display behaviour.
#8
#9
#10
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?
#11
#413192: Make taxonomy terms fieldable and maybe this #412518: Convert taxonomy_node_* related code to use field API + upgrade path
#12
@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
#13
seems related to #410464: Menu name = node title upon node edit through user interface
maybe try to find 1 solution for all cases?
#14
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'?
#15
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."
#16
I like the wording in #15, it's much clearer for the end-user
#17
#18
Changed the text almost accordingly ... "for constructing" => "to construct" and fixed underscore replacements by hyphen.
#19
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.
#20
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.
#21
#22
#20 issues applied.
#23
UX wise thumbs up.
#24
+ 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.
#25
* 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
#26
I am going to mark this RTBC, hasn't gotten a review in 7 days. And doesn't seem like any objections are made.
#27
+// 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?
#28
comments removed.
#29
xrefs
- #522458: Reuse auto-machine-readable.js for content_types.inc
- #522462: Reuse auto-machine-readable.js for taxonomy.admin.inc
#30
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
#31
- 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
#32
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' => '-',
...
);
#33
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.
#34
Subscribe
#35
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.
#36
- Renamed js to machine-readable-value.js
- adjusted menu.admin.inc accordingly
- Wrapped t() around 'URL path'
#37
I like it now. Go, Clemens!
#38
Going to mark this RTBC, but sun will review.
#39
+ '#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 -0000That 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_menuWrong 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.
#40
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'.
#41
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!
#42
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).
#43
This patch converts node types to use the same mechanism
#44
I hope tha_sun can do a final review, and then this should totally go in!
#45
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:
#47
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
#48
Oke, tha_sun reviewed, dmitrig corrected and reviewed, and stella. I dont know what more we can do, lets get this in!
#49
There you go.
#50
sun's comment changes in the js file look good. Back to RTBC.
#51
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.
#52
Re-tested menus and content types, it does indeed still work.
#53
The last submitted patch failed testing.
#54
Rerolled patch #49, looks like webchick commited some code to content_types.js on 21st Aug.
Attached images, all seems to work ok
#55
Ready to fly.
#56
Looking good
#57
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.
#58
- #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. :)
#59
Great! Committed to HEAD. :)
Please make note of this new convention and how to implement it in the module upgrade guide.
#60
I'll re-open this issue after #550572: CSS+JS regressions related to form-item-[name] has landed.
Marked the following issues as duplicate of this one:
#477020: Content type machine readable name field does not recieve focus
#183180: Start using internal (machine readable) name for vocabulary
#227052: DROP: when the user adds a content type, base the type on the name using javascript
#471242: Abstract out [edit] interaction on add content type
#522458: Reuse auto-machine-readable.js for content_types.inc
#522462: Reuse auto-machine-readable.js for taxonomy.admin.inc
#61
Strange category.
#62
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.
#63
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.
#64
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?
#65
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.
#66
Tested on taxonomy add/edit vocabulary, works as expected
attached screenshots
#67
I can only guess that you forgot to change the status to RTBC then. ;)
#68
ooops, yes, sorry for that. RTBC! :)
#69
tagging
#70
The last submitted patch failed testing.
#71
HEAD is broken
#72
Yay removing crufty code!
Committed to HEAD.
#73
Automatically closed -- issue fixed for 2 weeks with no activity.
#74
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?