Issue Details (XML | Word | Printable)

Key: MDL-19756
Type: Sub-task Sub-task
Status: Open Open
Priority: Minor Minor
Assignee: moodle.com
Reporter: Tim Hunt
Votes: 0
Watchers: 2
Operations

Add/Edit UI Mockup to this issue
If you were logged in you would be able to see more operations.
Moodle
MDL-19077

convert the remaining weblib output functions to $OUTPUT methods

Created: 09/Jul/09 05:50 PM   Updated: 13/Nov/09 04:08 PM
Return to search
Component/s: Lib
Affects Version/s: 2.0
Fix Version/s: 2.0

File Attachments: 1. Text File confirm_dialog.patch (2 kB)
2. Text File outputlib.patch (125 kB)

Issue Links:
Dependency
 

Participants: Dongsheng Cai, Jordi Piguillem Poch, moodle.com, Nicolas Connault and Tim Hunt
Security Level: None
Affected Branches: MOODLE_20_STABLE
Fixed Branches: MOODLE_20_STABLE


 All   Comments   Change History   Version Control      Sort Order: Ascending order - Click to sort in descending order
Nicolas Connault added a comment - 14/Jul/09 05:34 PM
Attached a patch with the conversion of the helpbutton() function.

Nicolas Connault added a comment - 14/Jul/09 08:04 PM
Updated patch

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 - 15/Jul/09 05:07 PM
Attached a patch with migration of *_to_popup_window() functions.

Nicolas Connault added a comment - 16/Jul/09 10:05 AM
Updated the popup patch

Nicolas Connault added a comment - 20/Jul/09 11:21 AM
Just attached a general patch that includes all my changes.

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.

Nicolas Connault added a comment - 22/Jul/09 09:40 AM
Finished refactoring helpbutton

Tim Hunt added a comment - 22/Jul/09 02: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:
  • 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 02: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.


Nicolas Connault added a comment - 22/Jul/09 10:12 PM
Finished fixing all of the issues reported by Tim.
Also added YUI tooltips over helpbuttons.

Nicolas Connault added a comment - 23/Jul/09 11:15 AM
Attached a patch for only weblib.php, outputlib.php and deprecatedlib.php

Tim Hunt added a comment - 23/Jul/09 01: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 {code}
    if ($button->disabled) { $buttonattributes['disabled'] = 'disabled'; } {code}
    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

Nicolas Connault added a comment - 25/Jul/09 11:32 AM
Fixed all reported issues, Tim, plus did some more refactoring in various places.

Nicolas Connault added a comment - 25/Jul/09 06:19 PM
Deprecated print_paging_bar and notice_yesno, patch updated

Dongsheng Cai added a comment - 26/Aug/09 03:59 PM
Hi, Nicolas, I made a patch to make confirm_dialog support callback function, can you please review it?

Thanks


Jordi Piguillem Poch added a comment - 22/Oct/09 07:42 PM
Hi,
method textarea is still unimplemented. Will it be added to the output renderer?

Thanks
Pigui.