Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lisarex’s picture

Issue tags: +d7help

adding tags.

Also, quick review (will reroll if time permits)... all module names should be capitalized!

arianek’s picture

Status: Active » Needs review
FileSize
8.06 KB

manually cut this from the mondo patch

tobiasb’s picture

FileSize
2.99 KB

Status: Needs review » Needs work

The last submitted patch failed testing.

tobiasb’s picture

Status: Needs work » Needs review
FileSize
8.08 KB

missing </dl> in field_help

arianek’s picture

i just added all appropriate caps and split the main field help from the submodules (2 patches now) because chx wasn't sure that they should have help at all and i'm thinking that the main one might be commitable earlier. the main field help is all very object oriented sounding, but i'm not sure what to do with it... needs input from @webchick.

arianek’s picture

ps. we don't have a link to the handbook page for field.module yet, but i didn't want to make a stub page on d.o in case cck is going to just be turned into that. also needs input from @webchick or maybe @jhodgdon on that one.

jhodgdon’s picture

Status: Needs review » Needs work

I think that all core modules should have at least an About section. You can enable these modules separately... why not make a help screen for them?

The current patch for Field itself needs capitalization updates in places.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
3.35 KB
33.6 KB

Correction: I guess you cannot enable or disable those field modules... So I'm wavering on whether they should have help text.

Anyway, here's a new patch for the Field module, with updated text.

arianek’s picture

FileSize
3.4 KB

- found one bad verb tense, and one caps - added these, and rolled a new patch for Field module.
- patch for all of the submodules in comment #6 still stands http://drupal.org/files/issues/help_field_submodules.patch

IMPORTANT: concerns still not addressed from above:

- the main field help is all very object oriented sounding, but i'm not sure what to do with it...
- we don't have a link to the handbook page for field.module yet, but i didn't want to make a stub page on d.o in case cck is going to just be turned into that (not sure how that works). if this is added, it should be the final sentence of the About paragraph, and read "For more information, see the online handbook entry for Field module." with "Field module" linking to the handbook page.

jhodgdon’s picture

The main field module cannot really be used by a user directly. It's a programmer's module, really. So I think the tone is OK... but thenagain I'm a programmer...

I do NOT think we should turn CCK pages into Field pages. Creating a new one would be better. It doesn't work exactly the same, and the API is quite different even if the UI is similar.

jhodgdon’s picture

FileSize
34.58 KB
3.54 KB

OK. I changed this around, I think you'll like the new version's tone better.

Again, this patch is just for the Field main module; sub-modules are in http://drupal.org/files/issues/help_field_submodules.patch on #6 above.

arianek’s picture

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

yay, love it. found one little code mistake (missing * in comments, prolly old) and a typo, otherwise lovely. and also added a link to new stub page in the handbook.

reposting both the field, and separate field submodules patch (if @webchick sees fit to commit it) for RTBC

webchick’s picture

Status: Reviewed & tested by the community » Active

Like Update status module, this one is a bit more technical in its wording than others. But as jhodgdon points out, it is at its heart a programmer module, so this tone is probably fine in this case. I went ahead and committed help_field9.patch to HEAD.

The submodules patch though, I'm a less enthused about. We already conveniently itemize the list of fields in the field module help text and I think that's much better from a usability standpoint than clicking on 50 different links. OTOH, when you download date module in contrib, you're going to get a help page for it, and then it'll be weird that you don't get one for these others. Hrm.

But I also don't see the point in committing the patch as-is when it has just a single line that repeats what was already in the Field module help. :P

Field SQL storage module too is a tricky one. That one is *completely* in the dev space and nothing user-facing at all. Since the help here is geared primarily toward site builders, I kind of lean toward not providing any help since it'd do more harm than good, probably. But... not sure.

So I'm setting the issue back to active so we can discuss the sub-modules more. I don't feel like we've quite reached a consensus yet.

jhodgdon’s picture

A few thoughts:

- Apparently, Drupal is no longer even showing the required modules on the Modules page (WTF? Who came up with that idea?). Modules field, list, number, field sql, etc are all marked required. So an admin will not really be aware they have them turned on, and has no way to disable them. I think?

- I can't think of anything to say for the list, number, and text sub-modules beyond the one-liners that are already there (and duplicated in the field module). Another alternative would be to put them into a paragraph/DD in Field, and link to the help files for the relevant modules. Something like this:

Enabling field types
The Field module provides the infrastructure for fields and field attachment, but the field types themselves are provided by additional modules. Some of the modules are required; the optional modules can be enabled from the Modules administration page. Drupal core includes the following field type modules: Text (required), Number (required), ...

- If someone comes up with some other field storage modules, then I think we would want help for SQL Storage in order to explain what it does specifically, since (one would hope) the other field storage modules would also have help to explain their benefits. Maybe we should mention field storage in the main Field help and link to SQL storage module as one alternative, to set the context? Or maybe not. Do we think other field storage mechanisms will be forthcoming?

jhodgdon’s picture

Status: Active » Needs review
FileSize
8.08 KB
6.28 KB
7.41 KB
11.4 KB
9.08 KB
25.7 KB
9.97 KB

OK, here's a proposed patch for Field and its submodules. Basically moving all the information into the submodule pages, and linking to submodules from the main Field help.

jhodgdon’s picture

As a note, the Field Storage SQL module already had a hook_help() implementation in it, so if we decide to leave Field as it is an want to not have help for Storage SQL, we would need a patch to remove that function.

webchick’s picture

Oh, now that's interesting. What if we took this one step further and looped through all of the available field types in the system (module_invoke_all('field_info')) and printed them out in a bulleted list, with links to their help pages if they exist? Then the Field help page could serve as a really useful reference point for all available field types in the system.

The decision to hide required modules from the modules page was made a long time ago by the UX team. I think the idea was that there's nothing you can do with those choices, so why bother eating up space with them? I agree though that it's sub-optimal here. It might be worth revisiting that discussion to see if we want to change this now that we have permissions/help links from the modules page.

jhodgdon’s picture

Yes, I think we should revisit that discussion.

Regarding a bulleted list, that is not a bad idea, but wouldn't that give us a list of fields rather than a list of modules that contain fields? I guess we could do a foreach( module_implements() ) or something like that, though.

Let me see what I can do with that...

jhodgdon’s picture

FileSize
10.58 KB
30.75 KB

OK, here's a new patch, and a new screen shot for Field (sub-module screen shots are same as above). This was a good exercise -- pointed out that Taxonomy is another core Field module. :)

Note that due to #563390: Seven theme lacks formatting on common html elements such as lists, paragraphs & <code> the bullet list formatting is not working in the screen shot. Sigh.

Also, I decided that the list should include widget modules as well as field modules. Hope that's OK.

arianek’s picture

I really like the direction is going on this, for what it's worth. :-)

But I'll also add my WTF to hiding the core required modules. There are so many reasons I think this is a bad idea, I won't even start listing them here... but seriously, would suggest that needs to be reevaluated!

jhodgdon’s picture

This is the issue:
#293223: Roll back "Hide required core modules"

I have just reopened it.

Issue tags: -d7help

jhodgdon requested that failed test be re-tested.

Status: Needs review » Needs work
Issue tags: +d7help

The last submitted patch failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
10.58 KB

I was afraid that patch wouldn't fly. Here's a reroll.

jhodgdon’s picture

FileSize
10.54 KB

Minor typo there, try this one.

arianek’s picture

- not a fan of the "but" in field help - maybe end the sentence or a semi-colon? "The Field module provides the infrastructure for fields and field attachment, but the field types and input widgets themselves are provided by additional modules."
- "drupal.org" should be "Drupal.org"
- still don't have this code wrapping all sorted, but i think this line is too long (over 80) "+ // Make a list of all widget and field modules currently enabled, in order"

otherwise thumbs up

arianek’s picture

errr..... one more thing, the first field type in the generated list that has been added to the field help page "file" isn't displaying as a link

jhodgdon’s picture

#28: It will only display as a link once the File help is committed. The links are to the help pages, and if the help page doesn't exist, it displays without a link.

#27: Really, we are saying "Drupal.org"? Code wrapping - correct, need to fix that. "but" -> ";" agreed.

jhodgdon’s picture

FileSize
10.54 KB

RE #29, yes you are right, I guess we are saying Drupal.org. Live and learn.

Anyway, here's a patch that fixes two instances of but (replace with ;) and the code wrapping and the Drupal.org fix.

By the way, all of those except the code line wrapping were in the previusly committed version of the module help. :)

arianek’s picture

Status: Needs review » Reviewed & tested by the community

that doesn't surprise me - fresh eyes catch new things now that we're not in full on crunch mode. ;-)

ya i had to look up the Drupal.org a while back, but that be the rule! being that the link will work once committed, i think that covers it all - looks good good RTBC

matason’s picture

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

Used theme_item_list instead of manual construction of ul list of field types.

Status: Needs review » Needs work

The last submitted patch failed testing.

matason’s picture

FileSize
9.31 KB

I ran cvs diff in the Drupal root directory this time, hopefully that will sort the fail.

I've re-worked the way the enabled field and input widget modules list is constructed, I found array_merge() left me with duplicate values in the array (because of numeric keys), I used array_unique() to sort that out, is there a more elegant approach?

I also used sort() to sort the array on values.

matason’s picture

Status: Needs work » Needs review

Setting to "needs review".

webchick’s picture

Status: Needs review » Fixed

I went to the testing bot page and it showed green, tho it hasn't been reflected yet here.

Thanks a lot for cleaning up the implementation there, matason!

This looks great. Committed to HEAD.

jhodgdon’s picture

Status: Fixed » Needs work

The patch that was committed didn't include the help screen for the Number sub-module for some reason (which was in #30).

matason’s picture

Status: Needs work » Needs review
FileSize
1.15 KB

Good catch, attached patch has hook_help() for the number module.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, works, etc. Let's get it in.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to HEAD!

Status: Fixed » Closed (fixed)
Issue tags: -d7help

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