Forums

autonav refactoring

Permalink
Hi there,

I'm really enjoying getting in to concrete5 and am starting to customise autonav somewhat (using templates at this point). After digging into it a bit I'm finding that they rapidly become quite if/else-y and somewhat hard to decipher. My back ground is from the java world (for better or worse) and I'm missing a criteria/visitor style api for using autonav. In my experience with such an approach you don't end up with code like:
if ($thisLevel > $lastLevel) {
    echo('<ul class="qsAdd">');
} else if ($thisLevel < $lastLevel) {
    for ($j = $thisLevel; $j < $lastLevel; $j++) {
        if ($lastLevel - $j > 1) {
            echo("</li></ul>");
        } else {
            echo("</li></ul></li>");
        }
    }
} else if ($i > 0) {
    echo("</li>");
}


Such patterns (in my experience) also tend encourage more reuse and less cut & paste.

Has anyone considered this kind of thing or is it something that will 'never happen' (tm)? I'd look at it my self but the getNavigationArray method and friends are pretty scary for a newbie such as myself.

The style of code I was thinking of would be something like the following:

DISCLAIMER: The details of the following are all made up off the top my head. It's really just to give an idea of the feel of the approach. If things like $criteria.startAtRoot() etc don't make sense, then just ignore it or substitute what would.

// Define a criteria for the pages we're interested in.   
$criteria = new AutoNavCriteria(); 
$criteria->excludePagesWithAttribute('exclude_from_nav');
$criteria->startAtRoot();
$criteria->includeAllChildren();
$criteria->sortAscending();
$criteria->includeAttributeInResult('page_icon');
$criteria->includeAttributeInResult('some_other_attribute_I_need_to_render');
// Now execute the query...
// I'm assuming AutoNavItem is like the current AutoNavBlockItem but that is
// a tree rather than an array, and that has the methods visit & visitChildren.
AutoNavItem rootItem = $concrete5UtilThingy->executeQuery($criteria);
// Define our visitor that will actually render our menu structure.
class SiteNavBuilder implements AutoNavItemVisitor {
   public function visit($rootItem) {
Viewing 15 lines of 38 lines. View entire code block.


It would also be possible to create a DefaultAutoNavBuilder (implements AutoNavItemVisitor) class that could be used out of the box in many cases. Typically it'd have default methods like generateListItem(), generateAnchor() that could be overridden by subclasses.

Thoughts?

Cheers
Andrew
 
jordanlev replied on at Permalink Reply
jordanlev
Hi,
Nice to see some more advanced developers on here :)

1) I agree that the default template for many of the built-in blocks are quite messy. I think this is just due to legacy reasons (and now they "just work" and everyone is familiar with them so it would be bad to change them now). BUT the great thing about C5 is that it is very easy to swap out these templates. You can override the default one, or create other ones that can be chosen from a dropdown menu (search the docs and forums for "custom template").

2) Your approach is very Java-esque (for better or worse). There's no technical reason why that can't be implemented in PHP, but I would say that from a cultural perspective, it will 'never happen' (tm). Most php developers are using it as a templating language (whether they know it or not), and in the web development world I think this is actually a better way to do things, primarily because you so often have to communicate with designers who build the HTML/CSS but don't know code at all -- and there's no way they'll be able to wrap their heads around any kind of OO or visitor pattern.

So in my opinion, it's good to keep things as declarative-looking as possible. Yes it can get messy at times, but overall I think this is actually a HUGE benefit of Concrete5 over some other CMS's -- take a look at how Drupal implements templates and you'll see that their approach is much more programmer-oriented and modular, but in my opinion it makes it really difficult for designers to get pages to look the way they want.

All just my 2 cents of course.

-Jordan
andrewpietsch replied on at Permalink Reply
Howdy, thanks for getting back to me. Fully understand the legacy thing, I have lots of legacy code (c:

My thoughts were more about refactoring autonavs internals so it would be more readily usable by developers and not about changing whole template style. I've had to create custom templates (for something I'd rather create a custom block) for but it's getting quite hacky for various reasons: lots of if/else, database code in views, can't include the templates via a package so they have to go in `<installDir>/blocks` etc.

My initial thoughts would be to extract out a query api and have Autonav use that. The query would return a tree of NavTreeItems which would define a flatten method that can be used to generate the current array of AutonavBlockItems. Hence the AutonavController::getNavigation() would look something like this:
function generateNav() {
   $query = new AutoNavQuery();
   // populate criteria from block settings...
   $query->orderBy($this.orderBy);
   etc...
   // execute the query...
   $navTreeRoot = $query->execute(); 
   // and flatten the tree to the well known list structure...
   return $navTreeRoot->flatten();
}


This way everyone is non the wiser but those who want to customise things can do so with out the nasty if/else required for heirarchical nav structures. Acutally the flatten method should take a function that does the flattening (i.e. map/reduce style). In that case the flatten call would be:
$navTreeRoot->flatten($convertNavTreeToAutoNavBlockItemFunction);


I would also probably implement NavTreeItems so that they were lazy loading, I suspect then that most (all?) the recursive code in AutonavBlockController::getNavigationArray() wouldn't be required as each node would just query for it's children in the appropriate order (although this is wild speculation on my part).

An approach like this would be really nice since I'm adding custom page attributes to exclude stuff from menus (and others have done the same) and would like an elegant way to get at them. For example, the current code is forcing my templates execute additional database hits (i.e. hasChildren doesn't take into account if the children of the node are configured with "exclude_nav" etc). Hence in my template I have to go:

if ($ni->hasChildren()){
  // the hasChildren flag doesn't take into account the exclude_nav attribute $childIds = $_c->getCollectionChildrenArray(1);
  foreach ($childIds as $childId) {
    $childPage = Page::getByID($childId);
    if (!$childPage->getCollectionAttributeValue('exclude_nav')) {
        $liClasses[] = 'has-children';
        break;
    }
  }
}


Which violates the whole model/controller/view separation thing on so many levels! (c:

Cheers
Andrew
jordanlev replied on at Permalink Reply
jordanlev
I very much agree that there should be a system "API" for autonav. There's already one for "PageList" that is very clean and easy to use, but unfortunately it only brings up results in a single-level list (no hierarchies). Ideally I would think the hierarchical stuff could be added to the existing PageList object (it's already confusing enough to explain to users the difference between the Page List block and the AutoNav block).

BTW, unless I'm misunderstanding you, it IS possible to include templates in a package. I've made a few addons in the marketplace that are nothing more than custom templates for other blocks:
http://www.concrete5.org/marketplace/addons/page-list-teasers/...
http://www.concrete5.org/marketplace/addons/autonav-exclude-subpage...

(Or are you talking about additional templates for a theme?)

-Jordan
andrewpietsch replied on at Permalink Reply
Awesome about the package thing, thanks for that that. It's weird, I'd tried that and it didn't seem to work... I'd also thought I'd read somewhere that you couldn't do it... perhaps it was an old comment. I wonder what I did wrong...

Cheers
Andrew
andrewpietsch replied on at Permalink Reply
Ok, I figured out what my issue was. I was trying to override the default view from a package and that's something you can't do.
JohntheFish replied on at Permalink Reply
JohntheFish
this thread is a bit old. Its been overtaken byhttp://www.concrete5.org/community/forums/customizing_c5/new-cleane...
andrewpietsch replied on at Permalink Reply
Thanks for that, I hadn't seen that thread, I'll check it out.
andrewpietsch replied on at Permalink Reply
After needing a more complex navigation structure than I knew how to do with C5 (see http://www.concrete5.org/index.php?cID=210171)... I decided to take a crack at this and developed a small package for manual creation & manipulation of a site menu. It uses PageList under the hood to generate a navigation tree which can then be manuipulated and rendered using a Renderer.

There's a public repo with some documentation at https://bitbucket.org/pietschy/c5_navigation_builder/...

It's lightly unit tested. I have no idea how it would perform for deep navigation structures since it recursively uses PageList for each level but then so does the autonav controller.

Cheers
Andrew