Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
API page: http://api.drupal.org/api/drupal/includes--pager.inc/function/theme_page...
In the description to the function the "text" parameter missed.
Comment | File | Size | Author |
---|---|---|---|
#27 | 1222802-theme_pager_link_doc_3.patch | 2.42 KB | franz |
#25 | 1222802-theme_pager_link_doc.patch | 2.42 KB | franz |
#21 | 1222802-theme_pager_link_doc.patch | 1.13 KB | franz |
#19 | 1222802-theme_pager_link_doc_D6.patch | 1.13 KB | franz |
#17 | 1222802-theme_pager_link_doc_D6.patch | 1.16 KB | franz |
Comments
Comment #1
droplet CreditAttribution: droplet commentedComment #2
jhodgdonI don't think that description really covers what the 'text' variable is used for?
Comment #3
jhodgdonComment #4
jhodgdonComment #5
droplet CreditAttribution: droplet commentedComment #6
franzI think you need to wrap comments in 80-width lines, right?
Comment #7
jhodgdonYes, 80-character wrapping.
Also, I still don't think that description is correct. Please look at the whole function code. $attributes is a local variable in the function, so it shouldn't be mentioned in the doc. Or if you need help figuring it out, add a note and someone will I'm sure help.
Comment #8
franzI just noticed the information was not right.
If $attributes['title'] is empty, it just uses the $text to determine whether to use a predefined title (e.g. "Go to first page"), a title concerning page number (e.g. "Go to page number @number"), or just no title. However, it doesn't ever set the title to be $text.
Comment #9
franzComment #10
jhodgdonThat's better... the description has a typo in it ( $variables['atributtes']['title'] ), and I think it could use a little more detail. How about something like this:
The link text. Also used to figure out the title attribute of the link, if it is not provided in $variables['attributes']['title']; in this case, $variables['text'] must be one of the standard pager link text strings that would be generated by the pager theme functions, such as a number or t('<< first').
One other thing I just noticed for the next patch. In the 'attributes' item, could you change:
apply to a pager anchor tag.
to
apply to the pager link.
Thanks!
Comment #11
franzI liked that version.
Comment #12
jhodgdonLooks good, thanks! 8.x/7.x... then mark backport to 6.x please.
Comment #13
droplet CreditAttribution: droplet commentedIt seems not right.
If $text isn't a numeric and not in the array. it doesn't nothing.
Point them to theme_pager function would get more clear ideas.
Comment #14
franzMeaning? It does not use it for the title attribute only. I think the current explanation pretty much covers this.
I added a see also statement and fixed the symbol for '« first', nice catch
Comment #15
jhodgdonAgreed, the explanation does cover it by saying "in this case", meaning if it is going to figure out the title attribute. Nice catch on the << vs. special character, yes. 8.x/7.x... then mark backport to 6.x please.
Comment #16
webchickCommitted and pushed to 8.x and 7.x. Thanks!
Moving back to 6.x.
Comment #17
franzPorted.
Comment #18
droplet CreditAttribution: droplet commentedno $variables like Parameters in D6
Comment #19
franzMy bad.
Comment #20
jhodgdonPlease read the text at below the Upload field -- don't name your patches with the _D6 extension.
Review of latest patch:
Can you also fix the first line of the function doc so it conforms to standards?
http://drupal.org/node/1354#themeable (or see the D7 one-line summary)
Other than that, looks good.
Comment #21
franzWhat's wrong with the first line, really? Re-submitting D6 patch...
Comment #22
jhodgdonDid you look at the link I posted for standards on what the first line should be for themeable functions? The existing first line on this function doc does not follow the standard.
Comment #23
franzYou mean "Returns HTML for a" ? That doesn't look mandatory, specially because this has been lying around on D6 for a long time already...
Comment #24
jhodgdonEverything on that page is mandatory - it is the standards page, not the suggestions page. :)
That doesn't mean that the standards are currently being followed by all docs (especially in D6), but when we fix docs for a particular function, we try to bring that function's doc up to standards.
Comment #25
franzThen it is better to have all of the other functions corrected as well to keep consistency. =)
Comment #26
jhodgdonIf you are going to fix all of them (which is a nice idea), we will need to bump this issue back up to Drupal 7.x/8.x, since they are likely also wrong there. The other option (recommended) is to leave this issue as D6 and fix only the function in question.
Also: Return -> Returns. You got the first one right, but not the rest.
Comment #27
franzD7/8 already contains this fix on docs.
Fixed spelling
Comment #28
jhodgdonyou're right, it does! Sorry about that.
OK, looks good for Drupal 6 -- brings the D6 doc for these functions up to the same standard as Drupal 7/8, and also adds the missing 'text' argument to the one function. Thanks!
Comment #29
Gábor HojtsyThanks, committed, pushed.