Moodle

I can't figure out how to assign this, but it should go to Tim Hunt.

Preliminary patch. Implements basic functionality.

Overview:

lib/javascript-static.js

• performance improvements to getElementsByClassName(), also handled case when class name parameter contained spaces.
• block_tree object for tree javascript

blocks/moodleblock.class.php

• block_tree class
• various tree_item classes

blocks/tree_test/

• an example of the block, probably should go in contrib

lib/outputlib.php

• new method moodle_renderer_base::tree_block_contents()

theme/standard/styles_layout.css

• styles for block

Sam, do you want to take a look at this, since you will probably be doing the navigation blocks. Looks pretty good to me at first sight.

Alan, thank you for doing this. As I said above, it looks promising.

A couple of minor things:

1. Which browsers have you tested this on? It would be useful to have a note of that here.

2. Not all methods have comments as required by the coding guidelines: http://docs.moodle.org/en/Development:Coding_style#Documentation_and_comments, and some of the ones that are there do really explain the API.

For example, if I want to make a block_tree subclass, what sort of thing do I need to put in $this->contents->items? Sure I can probably work this out from the code and the example (including it was a good idea) but I should not have to.

http://phpdocs.moodle.org/HEAD/moodlecore/theme_config.html is hopefully an example of a well-commented class.

Thanks for the comments Tim. I'm not working in my normal office today, but I'll work on this on Monday. For testing, I've only used Firefox up till this point, it probably works everywhere but I haven't tested.

One thing I would like is any enhancement requests. n.b. I'm intentionally leaving out icons because that should be in the CSS.

Ah, I had not noticed about the icons. I think that is a bad idea. At least inconsistency is a more of a bad idea than icons in CSS is a good idea. For now the block_tree class should be consistent with the block_list class, and later, if we are going to change them, we should do both in the same way.

Also, no matter what the final HTML looks like, the block instance had to give some information about the 'type' of each item that can be used to determine which icon is displayed. Now that can either be a CSS class name, or a image path name. If you look at the $OUTPUT->old_icon_url, then you will see that to specify an icon, you now just use a string like 'i/edit', 't/expand' and so on. I think you should use string like that, and the output function can then either convert them to img tags or class names in the HTML, but by default we should use img tags, for consistency.

I hope that is OK?

Another patch. There's more documentation in this one. It still needs testing though.

Changes:

lib/outputlib.php and lib/javascript-static.js

• reworked the output and javascript to make it much less convoluted and also more efficient.

blocks/moodleblock.class.php

• added tree_item_icon_text and tree_item_icon_link for using icons. They kind of look funny though because the "t/collapsed" and "t/expanded" icons are still there too.

I'm wondering if you really need 4 classes of list item and a base class. What happens if you just have one class, that has $text, that takes a fragment of HMTL, and an optional $icon that may be null?

I thought about that. I'm not particularly happy about having 4 classes, (especially the last 2, multiple inheritance would make that much nicer).

The reason I did it like this is:

1. When I went to write the block, I first wrote the test block. I tried to make it as simple as possible. Then I wrote code the block class. Also, at that time I wasn't thinking about icons (as content).
2. I don't like passing around HTML. It means whoever is implementing the code needs to bother with escaping things properly. It's nice to control that sort of thing in one central location. Additionally, there's a separation of data from the way that the data is arranged.

Another patch. I've come up with a solution I like a lot better (basically followed your advice).

The tree_item class is now all that is needed. It's constructor takes HTML and the icon is a property of the object. One thing that bothered me before was that these constructors were taking too many arguments and trying to differentiate between them in large amounts of code was a big pain.

The tree_item_text and tree_item_link classes still remain. They're just small wrappers around tree_item. I think tree_item_link makes code a bit cleaner, but they can easily be removed if the extra code isn't worth it.

The tree_item_icon_text and tree_item_icon_link classes have both been canned.

Getting better all the time. I really don't think the subclasses add anything.

If you really wanted to offer that helper code to create tree_items it would be better to use factory methods. That is

class tree_item {
    // ...
    public static make_link_item($text) {
        $title = isset($title) ? " title='".s($title)."'" : '';
        $html = sprintf("<a href='%s'%s>%s</a>", s($href), $title, s($text));
        return new  tree_item($html);
    }

However, I would not bother with this at all.

Hi all,
Just letting you know that as part of the global navigation proposal I have created a mock up to gather feedback on the proposed structure.
There is a forum discussion with details and to collect people's comments at: http://moodle.org/mod/forum/discuss.php?d=128359

The stuff you have come up with so far Alan is looking real good by the way, can't wait to see this all in action.

I think this code was used to build the Moodle 2.0 navigation, and so this bug can now be closed. If I am wrong, please re-open.