Tim Hunt
added a comment - 14/Jul/09 10:15 PM Should we allow $linktext to be true/false/"Some text" so you can have different visible text and tooltip. Hmm. or is it better to have no tooltip when there is visible text?
We should clean up $tooltip = get_string... to not concatenate strings.
Does the code become cleaner if we use moodle_url?

Nicolas Connault
added a comment - 21/Jul/09 11:49 PM Attached another more recent patch. Be mindful that some of the API details are still under consideration, particularly the abstraction details of action_icon and help_icon.

Tim Hunt
added a comment - 22/Jul/09 2:04 PM More review comments:
You've got a bit of trailing whitespace in deprecatedlib.php
I am not going to criticise duplicated code in deprecatedlib.
I don't like html_spacer::br. Surely the only place we want that to be used is in deprecated print_spacer, so don't add it to the UI. Just do
if ($br) {
$output .= '<br />';
}
In the deprecated function for backwards compatibility.
print_textarea has an incomplete comment "* When using this function, you should"
Actually, is print_textarea finished? I assume not.
Looking at prepare_event_handlers:
YAHOO.util.Event.preventDefault does not work like that! You need to call it in the JavaScript code, passing the event object that was passed to the even handler. An example of the correct usage is in http://cvs.moodle.org/moodle/mod/quiz/quiz.js?view=markup
I don't think this method should be on moodle_core_renderer anyway.
moodle_core_renderer::link does not do id parameters, even though a html_component may have one.
moodle_core_renderer::button still using the onclick attribute?
I think prepare_url should be in weblib.php, next to the moodle_url class.
prepare_url has trailing whitespace on some lines.
Why does prepare_url do
// Clean params
foreach ($url->params() as $var => $val) {
$url->param($var, s($val));
}
moodle_url already escapes ouptut correctly, and anyway that code has not effect.
prepare_url needs unit tests!

Tim Hunt
added a comment - 22/Jul/09 2:37 PM Lots more comments.
In several places you have incomplete PHP docs like "* @return string".
Elsewhere I have said "* @return string the HTML to be output."
action_icon should not call $link->prepare(); or $image->prepare();. $this->image and $this->link do that.
The previous comment applies in lots of other places too. I think you need to review all ->prepare()
link_to_popup has PHP doc that talks about button.
link_to_popup should use prepare_url rather than its own code.
The way you have laid out arrays like $tagoptions = array( does not match the coding guidelines.
Since we already have JS calling openpopup, we don't need the "this.target='$popup->name'; "; bit. (In several places.)
I don't think $popup->image makes sense.
I hate the excessive whitespace indent on comments like (user_picture)
* @param object $userpic Object with at least fields id, picture, imagealt, firstname, lastname
* If any of these are missing, or if a userid is passed, the database is queried. Avoid this
I don't think the Note: on user_picture is clear. Instead I would write
This method can be used in two ways:
<pre>
// Option 1:
$userpic = new user_picture();
// Set properties of $userpic
$OUTPUT->user_picture($userpic);
// Option 2: (shortcut for simple cases)
// $user has come from the DB and has fields id, picture, imagealt, firstname and lastname
$OUTPUT->user_picture($user, $COURSE->id);
</pre>
I don't think user_picture needs to clone clone($userpic). (But I might be wrong)
I don't see any benefit in adding class="link" to every link. Just bloat.
Should help_icon::prepare complain if the user tries to set ->url manually?
In html_action_component. I think we need to just choose one API, $jsfunction/$jsfunctionargs or $onclick. Giving a choice is just confusing.
Why isn't html_action_component a html_action_component?
In html_image, why not just do public $alt = HTML_ATTR_EMPTY; makes prepare() smaller.
In html_image::prepare,
if (empty($this->title)) {
$this->title = $this->alt;
}
is jsut wrong. There are many cases when an image should have alt text, but no tooltip.
user_picture needs PHPdoc comments. In particualr I don't understand why we need both $link and $url. (You could allow $link to be true (for default link), flase for no link or a url.
Do not take the number of comments as an indication that this patch is bad. Quite the contrary, it is close, but not quite there yet.

Tim Hunt
added a comment - 23/Jul/09 1:20 PM More review comments.
No HTML in lang strings. Also, playing around, I think it works better if you do
$string['clickhelpiconformoreinfo'] = '... continues ... Click the help icon to read the full article.';
echo shorten_text($output, 400, false, '<span class="readmore">' . get_string('clickhelpiconformoreinfo') . '</span>');
and a rule in the theme for .readmore to be italic.
Help tooltip should have a 'loading' animated gif while the content is loading.
$this->page->requires->yui_lib('container'); should not be in header, and $this->page->requires->js_function_call('init_help_icons'); should not be in footer. Instead they should both be in
prepare_event_handlers needs to be fixed as above ($this->page->requires->yui_lib('event'); is already in required_event_handler, and YAHOO.util.Event.preventDefault does not work like that.)
Set a default of null for $link->title etc. so don't need to check for if (!empty($link->title)) { everywhere.
$button->prepare(); should set $button->disabled to '' or 'disabled', so
if ($button->disabled) {
$buttonattributes['disabled'] = 'disabled';
}
can be just
$buttonattributes ['disabled'] = $button->disabled;
the logic
if (!empty($button->title) && !empty($popup->title)) {
$buttonattributes['title'] = $popup->title;
}
looks wrong.
I don't think $linktext should be a separate parameter to action_icon. Should be a property of the class
In link_to_popup, the code
if (empty($popup->url)) {
$popup->url = $link->url;
}
is duplicated.
spacer has incorrect PHPdocs ( with optional line break.)
I don't think html_action_component is-a moodle_html_component It is more like a moodle_url, that is, a separate class that is sometimes used as an attribute of a moodle_html_component

Martin Dougiamas
added a comment - 30/Apr/10 10:06 AM Where was textarea dealt with? I cant find a replacement function in $OUTPUT. I'm removing the debugging notice in print_textarea until we have one.