Support from Acquia helps fund testing for Drupal Acquia logo

Comments

surendramohan’s picture

Assigned: Unassigned » surendramohan

I am looking into this issue.

surendramohan’s picture

Status: Active » Needs review
FileSize
15.28 KB

Updated:
"About" section
Added:
"Uses" section ie. "Managing select list" and "Managing checkboxes/radio buttons" section

Attached patch for your reference.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! However, This patch has a couple of problems:
- Includes unrelated stuff from System module.
- Has indentation problem.
- Does not follow URL guidelines:
https://drupal.org/node/632280#url-note

I also don't think that we actually want a Uses section here. This module cannot really be used by itself, and I think it will be confusing to have this help here. Besides, I don't really think the explanation here is necessary?

surendramohan’s picture

Thanks for your feedback along with your suggestions.
I will make the suggested modifications and update you with my latest version.

batigolix’s picture

Component: documentation » options.module
Assigned: surendramohan » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
Parent issue: » #1908570: [meta] Update or create hook_help() texts for D8 core modules
Related issues: +#2091311: Update hook_help for Contextual Link module
FileSize
15.78 KB
1.9 KB

Patch addresses points from #3 and adapts text to standard for field type modules set in Link module #2091311: Update hook_help for Contextual Link module

batigolix’s picture

batigolix’s picture

tiny improvement.

interdiff is against #2

batigolix’s picture

Removed some references to module that I copy-pasted from

jhodgdon’s picture

Status: Needs review » Needs work

I don't think this help matches the UI of the module. It seems to create 3 fields: List (integer), List (float), and List (text). There is not really a "list" field, so "The settings and the display of the list field" isn't really very accurate? And maybe the help should explain what the 3 field types are used for, and something about how you can have labels that do not match the stored values?

batigolix’s picture

Status: Needs work » Needs review
FileSize
1.43 KB
2.67 KB

This patch adds a description for the 3 different list types and a section on key - label options as suggested in #9

Text is based on the proposed help text for the Number module: #2091345: Update hook_help for Number module

jhodgdon’s picture

Status: Needs review » Needs work

This is mostly great, thanks! A few small things to fix:

a) As in the case of the Number module, I think we should refer to "the List fields" whenever we talk about the field(s), because there are several types and they are shown individually when you go to add a new field.

b) In a list with "or", we need a comma before the "or". (2nd Uses item).

c) I think the help text about choosing the type is a bit misleading, because what you are choosing is the value stored in the database, but it talks about letting the user choose this type of values from the list, and what they will actually see is the label. So maybe it would make sense to put the 3rd Uses item about option keys and labels in 2nd position, and then make it clear in the "choosing type" item that you are talking about the keys?

d) "When you define the list options you can define a key and a label. " Do you think this would be clearer if it said "... and a label for each item in the list"?

batigolix’s picture

Status: Needs work » Needs review
FileSize
2.66 KB
2.47 KB

Patch addresses point in #11

jhodgdon’s picture

Looking pretty close! A couple of suggestions:

a) First sentence of About:

... create fields for storing lists of items

I don't think this is really accurate? The information that is *stored* is not really a list, at least if the field is single-valued. You use a list for *selecting* a data value, but what is stored is a single value, not a list. Maybe it would be better to say:

The Options module allows you to create fields where data values are selected from a fixed list of options.

This will also mesh well with the 2nd uses item, which talks about how to define the options for the list. And maybe throughout we should make sure to always call them "options" rather than using other terminology?

b) In the last Uses item, I think it's still not quite clear that the type refers to what is stored rather than the label. Maybe the first sentence should say:

There are three types of list fields, which store different types of data:

And then at the end of that Uses item, add a sentence saying something like:

No matter which type of list field you choose, you can define whatever labels you wish for data entry.

jhodgdon’s picture

Status: Needs review » Needs work
batigolix’s picture

Status: Needs work » Needs review
Related issues: +#2091345: Update hook_help for Number module
FileSize
3.65 KB
2.81 KB

Fixed the two points from #13

Status: Needs review » Needs work

The last submitted patch, 15: drupal8x-update-help-options-module-2091347-15.patch, failed testing.

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs manual testing

This looks great to me!

It just needs a manual test:
- Verify that all the links work
- Verify that all mentions of pages/text within the UI match what is seen in the UI
- Verify that the formatting is OK.

I'll also hit "retest" because the test failures above were unrelated to this patch.

jhodgdon’s picture

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I gave this a manual review today and it all looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e6f381c and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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