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.
Comment | File | Size | Author |
---|---|---|---|
#66 | 01_new_vocabulary.png | 7.3 KB | sign |
#66 | 02_new_vocabulary_with title.png | 11.33 KB | sign |
#66 | 03_new_vocabulary_with title_edit_expanded.png | 14.33 KB | sign |
#66 | 04_vocabulary_with title_edited.png | 10.66 KB | sign |
#66 | 05_vocabulary_with custom_machine_name_edited.png | 13.99 KB | sign |
Comments
Comment #1
Bojhan CreditAttribution: Bojhan commentedWould require http://drupal.org/node/471242
Comment #2
clemens.tolboom#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
Comment #3
kika CreditAttribution: kika commentedProposed machine name description:
"The unique machine readable name for this menu, can only contain lowercase letters, numbers and hyphens."
Comment #4
jix_ CreditAttribution: jix_ commentedMockups!
Note also that, for consistency, I called the fields "name" and "machine name" instead of "title" and "machine name".
Comment #5
clemens.tolboomWhen editing the menu only the description is available. Why not the Title?
#273137: Split Navigation to User and Administration menu
Comment #6
clemens.tolboomPatch 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.
Comment #7
clemens.tolboomThere is some similarity with #503816: Make form elements expandable concerning open/display behaviour.
Comment #8
clemens.tolboomComment #9
kika CreditAttribution: kika commentedComment #10
catchvocabulary.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?
Comment #11
kika CreditAttribution: kika commented#413192: Make taxonomy terms fieldable and maybe this #412518: Convert taxonomy_node_* related code to use field API + upgrade path
Comment #12
clemens.tolboom@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
replace to hyphen for machine readable menu
replace underscore for vocabulary
replace as is for menu title/fieldset on node/add and node/edit
Comment #13
seutje CreditAttribution: seutje commentedseems related to #410464: Menu name = node title upon node edit through user interface
maybe try to find 1 solution for all cases?
Comment #14
Dries CreditAttribution: Dries commentedWhen 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'?
Comment #15
clemens.tolboomOld 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."
Comment #16
attiks CreditAttribution: attiks commentedI like the wording in #15, it's much clearer for the end-user
Comment #17
clemens.tolboomComment #18
clemens.tolboomChanged the text almost accordingly ... "for constructing" => "to construct" and fixed underscore replacements by hyphen.
Comment #19
kika CreditAttribution: kika commentedTested, 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.
Comment #20
Dries CreditAttribution: Dries commentedFirst 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.
Comment #21
clemens.tolboomComment #22
clemens.tolboom#20 issues applied.
Comment #23
Bojhan CreditAttribution: Bojhan commentedUX wise thumbs up.
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedshould use #attached_js instead. grep for examples. there is a mention of this in the upgrade docs as well.
Comment #25
clemens.tolboom* 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
Comment #26
Bojhan CreditAttribution: Bojhan commentedI am going to mark this RTBC, hasn't gotten a review in 7 days. And doesn't seem like any objections are made.
Comment #27
catchThese need removing.
Could you open a followup issue for removing vocabulary.js and content_types.js in favour of automachinereadable?
Comment #28
clemens.tolboomcomments removed.
Comment #29
clemens.tolboomxrefs
- #522458: Reuse auto-machine-readable.js for content_types.inc
- #522462: Reuse auto-machine-readable.js for taxonomy.admin.inc
Comment #30
clemens.tolboomHmmm 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
Comment #31
clemens.tolboom- 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
Comment #32
kika CreditAttribution: kika commented1. 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:
you could write
Comment #33
clemens.tolboomThe 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.
Comment #34
Jeff Burnz CreditAttribution: Jeff Burnz commentedSubscribe
Comment #35
Bojhan CreditAttribution: Bojhan commentedmachine-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.
Comment #36
clemens.tolboom- Renamed js to machine-readable-value.js
- adjusted menu.admin.inc accordingly
- Wrapped t() around 'URL path'
Comment #37
kika CreditAttribution: kika commentedI like it now. Go, Clemens!
Comment #38
Bojhan CreditAttribution: Bojhan commentedGoing to mark this RTBC, but sun will review.
Comment #39
sunNot sure whether it should be "lower-case". (I'm no native speaker)
"for this menu" => "for the menu" ?
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.
(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).
That should go into system.js, no? At least, all it does is to provide a behavior, and behaviors are provided by modules.
Wrong syntax for @see.
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.
Comment #40
clemens.tolboomThanks 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'.
Comment #41
yoroy CreditAttribution: yoroy commentedClemens, 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!
Comment #42
stBorchertBojhan 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).
Comment #43
dmitrig01 CreditAttribution: dmitrig01 commentedThis patch converts node types to use the same mechanism
Comment #44
Bojhan CreditAttribution: Bojhan commentedI hope tha_sun can do a final review, and then this should totally go in!
Comment #45
catchThis 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:
Comment #47
stella CreditAttribution: stella commentedThis 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
Comment #48
Bojhan CreditAttribution: Bojhan commentedOke, tha_sun reviewed, dmitrig corrected and reviewed, and stella. I dont know what more we can do, lets get this in!
Comment #49
sunThere you go.
Comment #50
catchsun's comment changes in the js file look good. Back to RTBC.
Comment #51
sunActually 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.
Comment #52
catchRe-tested menus and content types, it does indeed still work.
Comment #54
sign CreditAttribution: sign commentedRerolled patch #49, looks like webchick commited some code to content_types.js on 21st Aug.
Attached images, all seems to work ok
Comment #55
sunReady to fly.
Comment #56
Bojhan CreditAttribution: Bojhan commentedLooking good
Comment #57
webchickCouple quick observations that might or might not need to be addressed:
It looks like this needs to be run through check_something() since it's output to the page.
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.
Comment #58
sun- #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. :)
Comment #59
webchickGreat! Committed to HEAD. :)
Please make note of this new convention and how to implement it in the module upgrade guide.
Comment #60
sunI'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
Comment #61
sunStrange category.
Comment #62
sunwell, 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.
Comment #63
sunTests 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.
Comment #64
sign CreditAttribution: sign commentedTested, 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?
Comment #65
sunPlease 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.
Comment #66
sign CreditAttribution: sign commentedTested on taxonomy add/edit vocabulary, works as expected
attached screenshots
Comment #67
sunI can only guess that you forgot to change the status to RTBC then. ;)
Comment #68
sign CreditAttribution: sign commentedooops, yes, sorry for that. RTBC! :)
Comment #69
pwolanin CreditAttribution: pwolanin commentedtagging
Comment #71
lilou CreditAttribution: lilou commentedHEAD is broken
Comment #72
webchickYay removing crufty code!
Committed to HEAD.
Comment #74
Chris Gillis CreditAttribution: Chris Gillis commentedCurrently 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?