Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Navmap feature for nested navigation #81

Closed
wants to merge 31 commits into from

Conversation

jerger
Copy link
Contributor

@jerger jerger commented Jan 4, 2017

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).

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
Copy link
Member

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

@jerger
Copy link
Contributor Author

jerger commented Jan 10, 2017

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
Copy link
Member

Awesome, I'll take a look soon :)

@lacarmen
Copy link
Member

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
Copy link
Contributor Author

jerger commented Jan 16, 2017

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
Copy link
Member

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
Copy link
Contributor Author

jerger commented Feb 7, 2017

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

  1. on configuration scope an additional parameter {:navbar-model [:flat | :hierarchic]} - flat will be the default.
  2. on page scope the {:navbar? [true|false]} and I can remove {:navmap? }
  3. 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
Copy link
Member

lacarmen commented Feb 7, 2017

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
Copy link
Contributor Author

jerger commented Feb 7, 2017

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

@jerger
Copy link
Contributor Author

jerger commented Feb 10, 2017

Hi,

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

  1. on configuration scope an additional parameter {:page-model [:flat | :hierarchic]} - flat will be the default. Flat mode is backward compatible.
  2. on page scope we respect {:navbar? [true|false]} in flat & hierarchic mode also.
  3. 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
Copy link
Member

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

@jerger
Copy link
Contributor Author

jerger commented Mar 4, 2017

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:

  1. As we've something new I would propose to have a new name in params {:pages ... } ?
  2. I would leave :navbar-pages and :sidebar-pages empty in hierarchic mode?
  3. 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
Copy link
Member

yogthos commented Mar 4, 2017

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
Copy link
Contributor Author

jerger commented Mar 5, 2017

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
Copy link
Member

yogthos commented Mar 5, 2017

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
Copy link
Member

lacarmen commented Mar 5, 2017

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

@jerger
Copy link
Contributor Author

jerger commented Mar 5, 2017

Okay, great - I will start refactoring :-)

@jerger
Copy link
Contributor Author

jerger commented Mar 12, 2017

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

New Features

Support of hierarchic menus & page structures:

  1. on configuration scope an additional parameter {:page-model [:flat | :hierarchic]} - flat will be the default. Flat mode is backward compatible.
  2. 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>

Copy link
Member

@lacarmen lacarmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With :page-model set to :hierarchic:

  1. If :page-root-uri is empty then :pages is empty
  2. If i have
/1.md
/2.md
/2/22.md

then in the :pages output I get page 1 with 22 as a child as well as page 2 with 22 as a child. Only page 2 should have 22 as a child.

@jerger
Copy link
Contributor Author

jerger commented Mar 22, 2017

Okay ...

  1. I could reproduce & have allready fixed (see https://github.com/DomainDrivenArchitecture/cryogen-core/blob/805e369bb8cccb5a6ed731271deda77a495bd70f/test/cryogen_core/hierarchic_test.clj - lin 96)
  2. 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
Copy link
Member

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.

image

@jerger
Copy link
Contributor Author

jerger commented Mar 22, 2017

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
Copy link
Contributor Author

jerger commented Mar 22, 2017

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
Copy link
Member

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
Copy link
Member

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

image
image

@jerger
Copy link
Contributor Author

jerger commented Mar 31, 2017

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
Copy link
Member

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

@jerger
Copy link
Contributor Author

jerger commented Apr 7, 2017

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
Copy link
Contributor Author

jerger commented May 12, 2017

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
Copy link

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
Copy link
Member

@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.

@lacarmen lacarmen closed this Jan 31, 2019
@jerger
Copy link
Contributor Author

jerger commented Feb 1, 2019

is there some doc how this works now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants