Navmap feature for nested navigation

jerger commented 7 years ago

Navmap will allow nested navigation for pages. To

add a page to navmap, you've to add {:navmap? true} to pages meta.
define navigation hierarchy you can use directory-structure (first level will be in /pages/nav1.md, second level will be in /pages/nav1/nav11.md)
use navmap, you can
- access navmap-pages (sequence of pages) in your template, same as you've used navbar-pages and
- access navigation childs as :navmap-children in specific pages (eg. nav1 will contain a sequence of one page representing nav11).

lacarmen commented 7 years ago

Is this PR in working order? I noticed you opened it but added some more commits afterwards.

jerger commented 7 years ago

Yes it is. After my initial pull request I found it useful to add a sort-by :page-index. So order of entries now depends on their :page-index instead of path. You will find a test, that should explain the function.

If you like this feature, I will add documentation also.

lacarmen commented 7 years ago

Awesome, I'll take a look soon :)

lacarmen commented 7 years ago

It's not clear to me what behaviour you're expecting when you say this PR is supposed to allow for nested navigation in pages.

I installed your PR locally and with the :clean-urls? option set to true in the config.edn, the compiler already supports outputting pages in a hierarchy. When this option is set to false the compiler throws an exception.

I also don't think it's necessary to add the navmap-pages key to the params map. Pages should always be split between navbar-pages and sidebar-pages. A tree of pages should be included in one of these vectors like so:

[{:page-index 1
  :title      "Page 1"
  :uri        "pages/page1"
  :prev       nil
  :next       {:title "Navmap 1", :uri "/pages/nav1"}}
 {:navmap?         true
  :page-index      2
  :title           "Navmap 1"
  :uri             "/pages/nav1"
  :navmap-children [{:navmap?         true
                     :title           "Navmap 11"
                     :uri             "/pages/nav1/nav11"
                     :page-index      1
                     :navmap-children [...]
                     :prev            {:title "Navmap 1", :uri "/pages/nav1"}
                     :next            { first item in children }}
                    {:title      "Navmap 12"
                     :uri        "/pages/nav1/nav12"
                     :page-index 2
                     :prev       { last item in previous page's children }
                     :next       {:title "Page 2" :uri "pages/page2"}}]
  :prev            {:title "Page 1", :uri "/pages/page1"}
  :next            {:title "Navmap 11" :uri "pages/nav1/nav11"}}
 {:page-index 3
  :title      "Page 2"
  :uri        "/pages/page2"
  :prev       {:title "Navmap 12" :uri "pages/nav1/nav12"}
  :next       nil}]

The prev/next links don't necessarily have to work like a DFS, that was just the best model I could think of at the moment. If you have a better idea then I'm open to suggestions.

By including the page tree in one of the sidebar/navbar vectors like this, the trees can be interposed between "normal" pages and users can choose to display only the top level link or the children as well.

jerger commented 7 years ago

Hi,

thanks for having a review on my pull request and sorry that I missed some things ...

It's not clear to me what behaviour you're expecting when you say this PR is supposed to allow for nested navigation in pages.

I will add a usage example in your documentation project, so it's getting more clear, I hope. Planned to do this next week ...

I installed your PR locally and with the :clean-urls? option set to true in the config.edn, the compiler already supports outputting pages in a hierarchy. When this option is set to false the compiler throws an exception.

I will have a look on this. You're right, I'm using :clean-urls? true so I've false not in scope. Can you tell me, what :clean-urls? false changes in terms of unit-test input data? I will add a test and fix the issue.

I also don't think it's necessary to add the navmap-pages key to the params map. Pages should always be split between navbar-pages and sidebar-pages. A tree of pages should be included in one of these vectors like so:

I've chosen the navmap parameter in order to keep backward compatibility. But I can imagine, that an additional edn parameter will work as well.

At the moment navbar and sidebar vectors are working globally flat - am I right?

The prev/next links don't necessarily have to work like a DFS, that was just the best model I could think of at the moment. If you have a better idea then I'm open to suggestions.

What does DFS mean?

By including the page tree in one of the sidebar/navbar vectors like this, the trees can be interposed between "normal" pages and users can choose to display only the top level link or the children as well.

If you talk about including sidebar-pages vector on page level (instead of global compiler-level like it's right now), yes. I've already though about that. But at the moment I do not need sidebars. So in terms of YAGNI (you ain't gonna need it) I've not touched sidebar-pages.

lacarmen commented 7 years ago

Can you tell me, what :clean-urls? false changes in terms of unit-test input data?

Instead of having /pages/nav1/, /pages/nav1/nav11/... etc as the uri's they would all end in .html.

At the moment navbar and sidebar vectors are working globally flat - am I right?

If I'm correct in my assumption of what you mean by "globally flat" then yes, the navbar and sidebar vectors don't have any nesting to them at the moment.

I've chosen the navmap parameter in order to keep backward compatibility.

Backward compatibility can be kept without the navmap-pages parameter. If you ignore the children in a navmap page then you can still have a flat list of pages. Using the example from my previous comment, if you ignored the children in in the "navmap page" then what you'd get in your list of links is:

Page 1
Navmap 1
Page 2

What does DFS mean?

DFS means depth first search. It's a tree traversal algorithm.

If you talk about including sidebar-pages vector on page level (instead of global compiler-level like it's right now), yes. I've already though about that. But at the moment I do not need sidebars. So in terms of YAGNI (you ain't gonna need it) I've not touched sidebar-pages.

I think there's some confusion here. I'm talking about including navmap trees in the already existing navbar-pages and sidebar-pages vectors like the example I provided in my previous comment. When I say "the trees can be interposed between 'normal' pages" I mean you can display your list of pages like

Page 1
Navmap 1
Page 2

Instead of having just

Page 1
Page 2

and "Navmap 1" off somewhere else because it's not included in navbar-pages or sidebar-pages.

By "users can choose to display only the top level link or the children as well" I mean that with this implementation users can choose to show

Top level pages only	All page levels
Page 1 Navmap 1 Page 2	Page 1 Navmap 1 Navmap 11 Navmap 12 Page 2

jerger commented 7 years ago

Hi, sorry for the long delay ... there is a huge bunch of work on my desk.

Regarding the clean-urls issue I will implement a test and fix.

Regarding not introducing additional navmap-pages I agree - a less redundant solution would be better for sure :-).

We've

{:navbar? } and {:navmap? } on page level and
{:navbar-pages } and {:navmap-pages }on result-level. Both having very similar semantic.

With the current implementation following example (lets assume, each page contains {:navbar? true}

/Chapter1/
/Chapter1/sub11
/Chapter1/sub12

will result in

{:navbar-pages [{/Chapter1/}
               {/Chapter1/sub11}
               {/Chapter1/sub12}]}

This behavior we should not break, because someones site will probably relay on this.

But instead of having a parallel structure like navbar & navmap we can also introduce a configuration, toggling between flat and hierarchic navbar vector - as u suggested. In this case we would have

on configuration scope an additional parameter {:navbar-model [:flat | :hierarchic]} - flat will be the default.
on page scope the {:navbar? [true|false]} and I can remove{:navmap? }
on result scope we will have {:navbar-pages [{/Chapter1/} {/Chapter1/sub11} {/Chapter1/sub12}]} or {:navbar-pages [{/Chapter1/}]} depending on configuration (see 1.) . sidebare-pages will react in the same way and contain only top-level sidebar-pages or all sidebar-pages.

What do you think about?

Best regards, jerger

lacarmen commented 7 years ago

Just to be clear, by a hierarchical model you mean

{:navbar-pages [{/Chapter1/ 
                      :children [{/Chapter1/sub11} {/Chapter1/sub12}]}]}

and a flat model would be

{:navbar-pages [{/Chapter1/} {/Chapter1/sub11} {/Chapter1/sub12}]}

?

If so then I think this implementation sounds good.

jerger commented 7 years ago

exact ... so I will go ahead and come back with a solution :-)

jerger commented 7 years ago

Hi,

I think, I've now a working solution. In short, we've:

on configuration scope an additional parameter {:page-model [:flat | :hierarchic]} - flat will be the default. Flat mode is backward compatible.
on page scope we respect {:navbar? [true|false]} in flat & hierarchic mode also.
params now may contain hierarchic navbar-pages / sidebar-pages. Hierarchic pages may contain a sequence of children {:children ({:title "child-page", :layout "page.html", :content " <p>child</p>"}) }

I've also a templating Example (works on bootstrap-4):

<ul class="navbar-nav">
  {% for nav-page in navbar-pages %}
    <li {%ifequal page.uri nav-page.uri %} class="nav-item active" {% endifequal %}
      {%ifunequal page.uri nav-page.uri %} class="nav-item" {% endifunequal %}>
      {% if not nav-page.children %}
        <a class="nav-link" href="{{nav-page.uri}}">{{nav-page.title}}</a>
      {% endif %}
      {% if nav-page.children %}
        <a class="nav-link dropdown-toggle" href="{{nav-page.uri}}" 
          data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">{{nav-page.title}}</a>
        <div class="dropdown-menu">
          {% for child-page in nav-page.children %}
            <a {%ifequal page.uri child-page.uri %} class="nav-item active" {% endifequal %}
         {%ifunequal page.uri child-page.uri %} class="nav-item" {% endifunequal %} 
         href="{{child-page.uri}}">{{child-page.title}}</a>
          {% endfor %}
        </div>
      {% endif %}
    </li>
  {% endfor %}
</ul>

For me it works now with unclean urls also. What do you think?

BR, jerger

lacarmen commented 7 years ago

Thanks, I'll take a look in the next couple of days when I find some time. :)

jerger commented 7 years ago

Found a problem with my proposed structure for sidebar-pages. I've to rethink hierarchic sidebar-pages ...

jerger commented 7 years ago

Okay ... analyzed the problem with sidebar-pages I found. I'm afraid, I was to simple hearted making :sidebar-pages hierarchic also. On my first website-usage I found it useful, to have

intended pages:

{:uri '/pages/home' :navbar true}
{:uri '/pages/home/firstSidebar/page' :navbar false}
{:uri '/pages/home/secondSidebar/page' :navbar false}

In my implementation I found the surprising result:

navbar-pages: ({:uri '/pages/home' :navbar true})
sidebar-pages: ()

The result is traceable, because for sidebar pages the sidebar root-page is missing. I thought about introducing :nav-child & :sidebar-child keys on page level, but doing this, we will mix up the semantic of :navbar-pages and :sidebar-pages. :sidebar-pages will contain :nav-childs and visy versa - so I expect users will have no more clean distinction.

Because of this situation I decided to revert making :sidebar-pages hierarchic. So my proposed solution now looks like:

on configuration scope an additional parameter {:navbar-mode [:flat | :hierarchic]} - flat will be the default and is backward compatible.
on page scope we respect {:navbar? [true|false]} in flat & hierarchic mode. In hierarchic mode :navbar-pages will get an additionall entry {:children ( [sequence of child pages] )}.
params now may contain hierarchic :navbar-pages - only the top level pages will be part of :navbar-pages sequence.

I've also a templating Example for navigation (works on bootstrap-4):

<ul class="navbar-nav">
  {% for nav-page in navbar-pages %}
    <li {%ifequal page.uri nav-page.uri %} class="nav-item active" {% endifequal %}
      {%ifunequal page.uri nav-page.uri %} class="nav-item" {% endifunequal %}>
      {% if not nav-page.children %}
        <a class="nav-link" href="{{nav-page.uri}}">{{nav-page.title}}</a>
      {% endif %}
      {% if nav-page.children %}
        <a class="nav-link dropdown-toggle" href="{{nav-page.uri}}" 
          data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">{{nav-page.title}}</a>
        <div class="dropdown-menu">
          {% for child-page in nav-page.children %}
            <a {%ifequal page.uri child-page.uri %} class="nav-item active" {% endifequal %}
         {%ifunequal page.uri child-page.uri %} class="nav-item" {% endifunequal %} 
         href="{{child-page.uri}}">{{child-page.title}}</a>
          {% endfor %}
        </div>
      {% endif %}
    </li>
  {% endfor %}
</ul>

And in addition an example for usage of sidebar pages (part of boostrap-4 carousel):

{% for sub-page in sidebar-pages|filter-flat:@page.uri %}
   <div {% if forloop.first %}class="carousel-item active"{% else %}class="carousel-item"{% endif %}>
    <img class="d-block img-fluid" src="{{sub-page.carousel-image}}">
      <div class="carousel-caption d-none d-md-block text-left">
         <h1>{{sub-page.title}}</h1>
         <p>{{sub-page.abstract}}</p>
      </div>
    </div>
{% endfor %}

As you can see, for sidebar pages I need an additional filter (filtering sidebare pages one level below current pages uri) ... maybe there is a option to provide a selmer-filter in cryogen-core, but I'm not shure about that. What do you think?

lacarmen commented 7 years ago

It doesn't make sense to have a navbar page with sidebar children. All descendants of a root page should inherit that page's placement.

jerger commented 7 years ago

I often use the pattern top-page having many sub-pages. So on top-page I can show teasers for and with link to each sub-page.

Having this pattern, the top-page is often element of navbar, the sub-pages are often not.

In terms of git-directory you're right. They all are part of a well defined placement in directory tree. But If you split this directory-tree by a navbar? predicate, the two resulting trees may have holes.

With the idea "navbar is intended to be navigable" the navbar tree will probably have no holes. But for sidebar this idea is not mandatory. Sidebar-pages has only to be navigable from one specific (navbar-)page. So not every sidbar-page has to be reachable in a closed sidebar-tree.

lacarmen commented 7 years ago

the navbar tree will probably have no holes. But for sidebar this idea is not mandatory.

But then this behaviour is inconsistent between the navbar pages and sidebar pages. They're both collections of pages, so the structure of the data should be the same.

jerger commented 7 years ago

You're right - but I do not know, how to make them same. Solutions I tried thought are:

hierarchic sidebar collection - does not work as outlined above.
build up hierarchy inside of navbar-pages as child elements: We would have navbar-child and sidebar-child-collections in pages. But sidebar-pages will also be accessible through params sidebar-collection. So we would have no more clear sematic.

So I took the easiest solution - left sidebar-pages flat and use filter ...

Do you've another idea?

lacarmen commented 7 years ago

As I said before,

All descendants of a root page should inherit that page's placement.

If the root of a nav-tree is a sidebar page then all the descendants should be sidebar pages. If the root of a nav-tree is a navbar page then all the descendants should be navbar pages.

jerger commented 7 years ago

So you would accept to have two different types of children (:navbar-children / :sidebar-children) in one page?

lacarmen commented 7 years ago

A page will be either a navbar page or a sidebar page. ALL of its children will be of the same type.

jerger commented 7 years ago

that won't work. I can provide access of my current website - so we can discuss on real object, if you want. Maybe You can give me a hint on my example to bring our discussion further?

I can provide credentials by pm.

yogthos commented 7 years ago

@jerger

Just looking through the thread. If I understand correctly, you'd like to have the ability to show top level pages in the navbar at the top, and then display child pages for the current page in the sidebar.

It seems like using a single nested structure would be best here. For example, you could do something like the following:

:pages
[{:title "Page 1"
  :url "/page1"
  :children
  [{:title "Child 1"
   :url "/page1/child1"}
   ...]}
 ...]

Then in the template yo could iterate the top level pages in the navbar:

<nav>
{% for page in pages %}
  <a href="{{page.url}}">page.title</a>
{% endfor %}
</nav>

And in the sidebar, you'd do:

<nav>
{% for page in pages %}
  {% ifequal page.url current-page %}
    {% for child in page.children %}
      <a href="{{child.url}}">child.title</a>
    {% endfor %}
  {% endifequal %}
{% endfor %}
</nav>

Does this approach seem reasonable?

jerger commented 7 years ago

As the hole-problem comes from upfront dividing the nested structure into :navbar true and :navbar false fractions your proposal having the whole nested structure of pages in one structure would solve the issue. So that's a fine good idea :-).

But there remain some questions:

As we've something new I would propose to have a new name in params {:pages ... } ?
I would leave :navbar-pages and :sidebar-pages empty in hierarchic mode?
In backward compatible flat mode :pages will be empty?

In Terms of template I need nested top-level navigation and sidebar-navigation in addition. So I have to some kind of :navbar true filter ...

For top level navigation it may look like:

<ul>
  {% for page in pages|filter-navbar %}
    <li>
      {% if not page.children %}
        <a href="{{page.uri}}">{{page.title}}</a>
      {% else %}
         <ul>
          {% for child-page in page.children|filter-navbar %}
            <li><a href="{{child-page.uri}}">{{child-page.title}}</a></li>
          {% endfor %}
        </ul>
      {% endif %}
    </li>
  {% endfor %}
</ul>

For sidebar this can be:

<ul>
  {% for page in pages|filter-sidebar %}
    <li>
        <a href="{{page.uri}}">{{page.title}}</a>
    </li>
  {% endfor %}
</ul>

I would appreciate to implement these filters also in cryogene-core because it's likely that many people will need this filters. Is it possible to add filters on cryogen-core level?

What do you think about?

If you will accept such a solution I will go to refactor :-)

yogthos commented 7 years ago

I would recommend to simply use :pages in both cases. When the structure is flat, then you're just going to have a collection of maps that you iterate. The template logic would just be:

<ul>
  {% for page in pages %}
    <li>
        <a href="{{page.uri}}">{{page.title}}</a>
    </li>
  {% endfor %}
</ul>

You simply don't filter the pages in that case and everything is assumed to be top level.

For the hierarchic mode, I think that adding filters for navbar/sidebar would be sensible. It makes the template code more explicit about what it's doing.

jerger commented 7 years ago

On a greenfield I would appreciate just having :pages.

The only argument against is backward compatibility. If we introduce :pages and throw away :navbar-pages and :sidebar-pages every user will have to change his templates (maybe we can use schema from #89 in order to express deprecation instead :).

Should I really break compatibility?

yogthos commented 7 years ago

I don't think it's as much of a concern for existing users. If I already have my site working the way I like, usually I just keep it on the version of the engine I was using. I think it would be reasonable to add a changelog.md or something to document breaking changes.

lacarmen commented 7 years ago

That's fine, users can filter their pages on their own terms.

jerger commented 7 years ago

Okay, great - I will start refactoring :-)

jerger commented 7 years ago

Okay, I've refactored and tested with my websites ... here the result is:

New Features

Support of hierarchic menus & page structures:

on configuration scope an additional parameter {:page-model [:flat | :hierarchic]} - flat will be the default. Flat mode is backward compatible.
params now contain hierarchic pages. Pages is replacing no longer supported navbar-pages / sidebar-pages. Hierarchic pages may contain a sequence of children {:children ({:title "child-page", :layout "page.html", :content " <p>child</p>"}) }

Breaking Changes

Pages is replacing no longer supported navbar-pages / sidebar-pages. In order to realize navbar / sidebar functionality, you've now to write filters.

Examples

I've also a templating Example for navigation (works on bootstrap-4):

{% for nav-page in pages|filter-navbar %}
    <li class="nav-item">
        {% with nav-children=nav-page.children|filter-navbar %}
            {% if nav-children|empty? %}
                <a class="nav-link" href="{{nav-page.uri}}">{{nav-page.title}}</a>
            {% else %}
                <li class="nav-item dropdown">
                    <a class="nav-link dropdown-toggle" href={{nav-page.uri}} id=id="{{nav-page.title}}" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">{{nav-page.title}}</a>
                    <div class="dropdown-menu" aria-labelledby="{{nav-page.title}}">
                    {% for child-page in nav-children %}
                        <a
                            {%ifequal page.uri child-page.uri %} class="dropdown-item active" {% endifequal %}
                            {%ifunequal page.uri child-page.uri %} class="dropdown-item" {% endifunequal %} 
                            href="{{child-page.uri}}">{{child-page.title}}</a>
                    {% endfor %}
                    </div>
                </li>
            {% endif %}
        {% endwith %}
    </li>
{% endfor %}

and a template example using children as sidebar pages:

<div class="row">
    {% for sub-page in page.children|filter-sidebar %}
        <div class="col-lg-4">
            <img class="rounded-circle" src="{{sub-page.headline-image}}" alt="{{sub-page.headline-image}}" width="140" height="140">
            <h2>{{sub-page.title}}</h2>
            <p>{{sub-page.abstract}}</p>
            <p><a class="btn btn-secondary" 
            {% if sub-page.action-uri %}href="{{sub-page.action-uri}}" target="_new"{% else %}href="{{sub-page.uri}}"{% endif %} 
                  role="button">{{sub-page.take-action}}</a></p>
        </div>
    {% endfor %}
</div>

jerger commented 7 years ago

Okay ...

I could reproduce & have allready fixed (see https://github.com/DomainDrivenArchitecture/cryogen-core/blob/805e369bb8cccb5a6ed731271deda77a495bd70f/test/cryogen_core/hierarchic_test.clj - lin 96)
I can not reproduce in my test. Do you've an idea, what's different in your setting? Is your setting public or should I create a public example for testing?

lacarmen commented 7 years ago

Just install this snapshot and make a new cryogen project like I described. If you output the contents of :pages then you'll see the problem.

jerger commented 7 years ago

I've a public example here: https://github.com/DomainDrivenArchitecture/LearningCryogeneGen/invitations - with your described scenario working.

Tomorrow I will take a close look at your example in order to find the relevant diff ...

jerger commented 7 years ago

okay got it. your pages are:

1.md
1/11.md
about.md
another-page.md

which results in

page 1.md having one child 1/1.md
about.md has no children but it has a :prev to page1, which has its child 1/1
another-page.md has :prev to about.

So that's seems to be okay to me?

lacarmen commented 7 years ago

Ah my mistake, it's hard to parse all that text. That's fine then. I'll take a look at your fix for the first issue.

lacarmen commented 7 years ago

The compiler does not output any pages if you set the :page-model to :hierarchic. I have the same setup as before:

1.md
1/11.md
about.md
another-page.md

jerger commented 7 years ago

In order to debug your case - can u provide the pages & config.edn to me?

You can overwrite my example - I invited you as contributor here: https://github.com/DomainDrivenArchitecture/LearningCryogeneGen/invitations - but providing an archive of your pages is also fine to me ...

lacarmen commented 7 years ago

I said my setup is the same as before. Just refer to my previous comment.

jerger commented 7 years ago

tried out your layout here: https://github.com/DomainDrivenArchitecture/LearningCryogeneGen

For me this works , so I can not reproduce your error. Maybe I've other configuration settings or there are other differences? If you find the difference, I will continue investigation ...

jerger commented 7 years ago

How will you go on with this pull request? I can not reproduce your issue ... so I've no idea what can be the next step ...

harlanji commented 5 years ago

We seem to have hierarchical pages now, I use it in 0.1.64 (example). Is there some element of this ticket that isn't covered? Forgive my lack of deep comprehension of this thread; I read it a while back, around 0.1.60. I can invest the time to understand if there's something to do in addition to what's been done--I see other seemingly stale tickets.

lacarmen commented 5 years ago

@harlanji

Is there some element of this ticket that isn't covered?

I don't think so, new tickets can be opened if there's something lacking.

jerger commented 5 years ago

is there some doc how this works now?

cryogen-project / cryogen-core