Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Project: Drupal core » Documentation
Version: 8.x-dev »
Component: documentation » API documentation files

Moving to documetation queue (the file to be edited is in that git repository).

jhodgdon’s picture

Issue tags: +valid issue

Tagging so #1421874: [meta] Documentation Issue Queue Cleanup doesn't have to look at API docs issues.

jhodgdon’s picture

Issue tags: -valid issue +FAPI reference

tagging

David_Rothstein’s picture

Linking to the Drupal 8 version of this. The patch here could use similar text as that one does.

David_Rothstein’s picture

Issue tags: +Novice
shashikant_chauhan’s picture

I am unable to find starting point where I can make changes so that it is reflected in developer/topics/forms_api_reference.html

joachim’s picture

It's the file developer/topics/forms_api_reference.html in the repo for this project. 7.x-1.x branch, I assume.

shashikant_chauhan’s picture

Unable to find such file "forms_api_reference.html" in drupal 7.x repo. Not sure if I am looking at a right place or not.

joachim’s picture

Not in the Drupal repo, in the repo for *this* project. This issue is not under the Drupal project! Click the tab that says 'Documentation' that's just above the breadcrumb.

shashikant_chauhan’s picture

Status: Active » Needs review
FileSize
12.43 KB

Adding patch

shashikant_chauhan’s picture

FileSize
53.25 KB
44.55 KB

Screenshots after applying the patch.

xeM8VfDh’s picture

Thank you VERY much @joachim and @sun for pointing out this useful behavior!

jhodgdon’s picture

Can someone test the code in the proposed changes (see screenshot) and verify that it works correctly in a D7 form? Thanks!

joachim’s picture

Status: Needs review » Needs work

The adding of extra options looks ok, but there is a problem with the main form element that's being added: drupal_map_assoc() should not be used with translated values, as it means the stored keys will be different in different languages! Only the option labels should be translated.

(Also, is there any way that line breaks could be added to the source, as it's really hard to read it on very long lines.)

jhodgdon’s picture

Feel free to add line breaks to the file.

shashikant_chauhan’s picture

Status: Needs work » Needs review
FileSize
55.8 KB
44.86 KB
14.93 KB

@joachim, I have removed the function "drupal_map_assoc" from docs.
After applying patch.
Radios:
Radios

Checkboxes
Checkboxes

jhodgdon’s picture

Status: Needs review » Needs work

By splitting the code line up into multiple lines, your patch has added a bunch of whitespace that shouldn't be on the page. For instance, it should look like

t('whatever')

not

t ( 'whatever' ) 

There should not be spaces around the (). Same with [].

Also because of splitting the lines, it is very difficult to look at the patch and see what has actually changed from the old version. It looks like the whole thing is completely different, but it actually isn't.

So... please make a new patch. Thanks!

shashikant_chauhan’s picture

@jhodgdon, These whitespaces are added due to new line breaks. After removing those new line break it will look like t('whatever')

Eg: https://jsfiddle.net/shashi1028/e1Ltukn8/
Or should I create patch like

<span>1</span><!--
--><span>2</span><!--
--><span>3</span>

to remove spaces.

jhodgdon’s picture

Yes, I understood that the line breaks generated the extra spaces. Please remove them, rather than adding comments.

Why do I say this? Well, ... It is a general philosophy in the Drupal project as a whole, that patches should only address the actual problem in the issue. So, if you have an issue that says "We should add to this documentation an example showing how to set options", you should make a patch that does just that. Not a patch that also totally reformats that section of the file.

So, if you want to make a patch that reformats a file, you should use a separate issue to do that, not combine it with this issue.

Thanks!

shashikant_chauhan’s picture

Status: Needs work » Needs review
FileSize
12.55 KB
54.08 KB
44.47 KB

Here is the new patch after removing line breaks.

bbombachini’s picture

Status: Needs review » Reviewed & tested by the community

The patch applied evenly and seems good. Moving it to reviewed.

apaderno’s picture

Status: Reviewed & tested by the community » Needs review