Make a hierarchical menu in navstories #183

bennyslbs · 2016-10-31T15:43:24Z

The hierachical menu is based on the structure of the stories (permalink).

See: https://groups.google.com/forum/#!topic/nikola-discuss/Ry4LpSMY0Bg

The hierachical menu is based on the structure of the stories (permalink).

Kwpolska · 2016-10-31T17:16:12Z

v7/navstories/README.md

+```
+
+To exclude a single story from the navstories meny add the following
+metadata `.. hidefromnav: yep` (yep can be anything) to the story.


yes or True

Kwpolska · 2016-10-31T17:16:52Z

v7/navstories/README.md

-Proof-of-concept of a ConfigPlugin.  By request on the mailing list.
+Started as a Proof-of-concept of a ConfigPlugin.  By request on the mailing list.
+
+Changed to map the stories to a hierachical structure in the menu


Wait, how deep down does this go? Do you realise you can have only one level of submenus?

Kwpolska · 2016-10-31T17:17:07Z

v7/navstories/README.md

+
+Format of `NAVIGATION_LINKS_POST_NAVSTORIES` is identical to `NAVIGATION_LINKS`.
+
+Sorting and display names in menu can be controlled for top-level entries via `NAVSTORIES_CFG`.


no example for this

bennyslbs · 2016-10-31T17:41:17Z

Hi,
Thank you for the review.

See my comments inline

2016-10-31 18:17 GMT+01:00 Chris Warrick notifications@github.com:

@Kwpolska commented on this pull request.

In v7/navstories/README.md
#183 (review):
+NAVSTORIES_MAPPING = {
DEFAULT_LANG: (
   # Mapping "Toplevel in permalink" to "Visible text"
   # The order is as listed here, entries not listed here are included in the end,
   # example (remove initial #):
   #("b", "Boo"),
   #("f", "Foo"),
),
+}
+NAVIGATION_LINKS_POST_NAVSTORIES = {

Format just as NAVIGATION_LINKS, but content included after navstories entries
+}
+```
+
+To exclude a single story from the navstories meny add the following
+metadata.. hidefromnav: yep (yep can be anything) to the story.
yes or True

I will change to yes, I will also change the code so that no or False gives
the same result as if hidefromnav isn't included, or what is the normal
behavior for metadata with boolean value?

In v7/navstories/README.md
#183 (review):

@@ -1,3 +1,34 @@
This very simple plugin throws all the stories to the navigation bar.

-Proof-of-concept of a ConfigPlugin. By request on the mailing list.
+Started as a Proof-of-concept of a ConfigPlugin. By request on the mailing list.
+
+Changed to map the stories to a hierachical structure in the menu

Wait, how deep down does this go? Do you realise you can have only one
level of submenus?

Only one sub-level, i didn't mention that in the README,
But I can include this:
This plugin generates menus and one level of submenus (further sub-levels
are mapped to first level submenus).
WARNING: Support for submenus is theme-dependent.

In v7/navstories/README.md
#183 (review):

@@ -1,3 +1,34 @@
This very simple plugin throws all the stories to the navigation bar.

-Proof-of-concept of a ConfigPlugin. By request on the mailing list.
+Started as a Proof-of-concept of a ConfigPlugin. By request on the mailing list.
+
+Changed to map the stories to a hierachical structure in the menu
+based on the permalink structure (defaults to the directory structure
+of the posts).
+
+The menu entries inserted by navstories are inserted after entries from NAVIGATION_LINKS.
+Entries listed in NAVIGATION_LINKS_POST_NAVSTORIES are inserted after navstories entries.
+
+Format of NAVIGATION_LINKS_POST_NAVSTORIES is identical to NAVIGATION_LINKS.
+
+Sorting and display names in menu can be controlled for top-level entries via NAVSTORIES_CFG.

no example for this

No, it should be NAVSTORIES_MAPPING, and there is an example for that.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#183 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKrkZ07GhDqnvCB3jnNZI4hcGAHS6BXBks5q5iKygaJpZM4KlKK7
.

bennyslbs · 2016-10-31T17:42:57Z

Previous mail was sent twice by an failure, except a small fix, so the last one had this fix: This plugin generates menus and one level of submenus (further sub-levels are mapped to first level submenus). Br, Benny

Kwpolska · 2016-10-31T18:28:50Z

Please respond on GitHub, not via e-mail. As for the hidefromnav variable, no need for no or False support (check presence, not value)

bennyslbs · 2016-10-31T20:05:32Z

I have updated README.md
Code checking hidefromdev are checking for presence, so nothing changed
(I have also removed my first comment of the dublets).

Skips initial stories or pages from menu.

permalink of the story shall match one of the entries in NAVSTORIES_PATHS for the given language to be included in the menu.

bennyslbs · 2016-11-01T18:22:33Z

I have installed v. 7.8.1 and found that default location for stories have been changed to pages, commit df8d5d2 made navstories work well for both pages and stories.

Commit 5667017 lets navstories only add menu entries if initial path of permalink match an entry in NAVSTORIES_PATH.

I think that was all for now.

Kwpolska

How does the resulting menu look for a sample site?

Kwpolska · 2016-11-01T18:25:40Z

v7/navstories/navstories.py

@@ -38,12 +38,50 @@ def set_site(self, site):
        site.scan_posts()
        # NAVIGATION_LINKS is a TranslatableSetting, values is an actual dict
        for lang in site.config['NAVIGATION_LINKS'].values:
+            # Which paths are navstories active for for lang?


one “for”

Kwpolska · 2016-11-01T18:28:13Z

v7/navstories/navstories.py

-
+                permalink = p.permalink(lang)
+                for s in navstories_paths:
+                    if permalink.startswith('/' + s + '/'):


startswith can take a tuple.

navstories_paths = tuple('/' + s + '/' for s in navstories_paths) # above ... if permalink.startswith(navstories_paths):

Yes, but next I have to remove '/' + s + '/', I don't know how to do it without regular expressions, and navstories_paths are not regular expressions.

Make the check using a tuple, then use a for loop to figure out the longest string that the permalink starts with.

Or perhaps:

navstories_paths = tuple(('/' + s.strip('/') + '/') for s in navstories_paths) for p in site.pages: permalink = p.permalink() s_candidates = [s for s in navstories_paths if permalink.startswith(s)] if not s_candidates: continue # get longest path s = max(s_candidates, key=len) navpath = permalink[len(s):].split('/')

Use case:

NAVSTORIES_PATHS = {DEFAULT_LANG: ('pages', 'pages/foo', 'pages/foo/bar')}

I will change to your code.
Should p.permalink() not be p.permalink(lang)?

Kwpolska · 2016-11-01T18:29:23Z

v7/navstories/navstories.py

+                permalink = p.permalink(lang)
+                for s in navstories_paths:
+                    if permalink.startswith('/' + s + '/'):
+                        navpath = permalink[2 + len(s):].split('/') # Permalink format '/A/B/' for a story in s/A/B.rst


What do you mean here with the comment?

It was just a note when I started chaing the plugin, I will remove it.

Kwpolska · 2016-11-01T18:50:45Z

v7/navstories/navstories.py

+            if 'NAVSTORIES_MAPPING' in site.config and lang in site.config['NAVSTORIES_MAPPING']:
+                navstories_mapping = site.config['NAVSTORIES_MAPPING'][lang]
+            for sk, sv in navstories_mapping:
+                for i in range(len(new_all)):


This code looks like a mess. Could you make it more readable/understandable?

(Also, I’d drop that setting, it makes people too lazy)

First two lines:
I am checking if site.config['NAVSTORIES_MAPPING'][lang] is defined before using it, is there a better way to do it?

I can rename sk and sv to be more readable (sk -> map_key, sv -> map_text), would that help?

I don't like to remove NAVSTORES_MAPPING since this can control in which order the menu entries are included. I want the mapping from key to text because I like the folders to be without spaces and special chars (danish non-ascii chars, even when I am using UTF-8) but would like to have this in the menu text.

You could use TranslatableSetting on all your setting to make things less of a mess.

Do the rename. And add some comments to explain your range(len()) thing.

Aren’t you using post.title()? My point is, if you need that much customization for things (so page titles need to be different in nav and page), perhaps you’re better off making menus by hand, will be much simpler than messing with this plugin.

1: Is there a example on using TranslatableSetting, I have searched the plugins repository for it, but didn't find much help.
2: I will do that
3: I am using post.title(), but would like to get a deeper menu

Demo site.
Nav and A menu entries are autogenerated. A is not listed in NAVSTORIES_MAPPING, so it comes after Nav.
https://slbs.dk/n/
The conf.py and directory struture under src/ is here: https://slbs.dk/n/listings/

Look at nikola.py in core for some samples, and at the utils.TranslatableSetting class (which is well-documented).

bennyslbs · 2016-11-01T19:18:43Z

I have not a public demo site at the moment, I will create one.

- Using smarter loops - Minor changes TODO: - p.permalink(lang) ->p.permalink() - Usage of TranslatableSetting

bennyslbs · 2016-11-02T19:31:54Z

p.permalink(lang)
When p.permalink(lang) shall be changed to p.permalink(), what then with p.title(lang)?

Usage of TranslatableSetting
I have looked into nikola.py and utils.py, but I am not sure I understand how you would like it implemented. I will soon commit a proposal.

Kwpolska · 2016-11-02T19:36:33Z

p.permalink(lang)

Please use the lang argument.

TranslatableSetting

You can use those to avoid some checks when it comes to languages in settings.

navstories_paths = utils.TranslatableSetting('navstories_paths', site.config['NAVSTORIES_PATHS'], site.config['TRANSLATIONS'])

You can use navstories_paths(lang) to get a translation for lang, or default to the default language. It will follow the behavior of NAVIGATION_LINKS and other built-in translatable settings.

bennyslbs · 2016-11-02T20:37:08Z

Edit: Just fixed permalink(lang) - now the links don't link to default language :-).

I think the first loop over the configuration variables is the smartest way to accept:

Missing configuration
Variable created without translation support
Variable created with translation support
It is similar to what was done in nikola.py

The loop in the top of the lang loop could be skipped, but I think it is easier to read.
Edit: I will change usage of TranslateableSetting as you mentioned above.

Create menu structure navpath based on permalink without language prefix, and links in menu including language.

But read config variables in a try...except in case a varible is missing.

bennyslbs · 2016-11-02T21:41:02Z

I had not seen your previous comment - I have just incorporated it, but reading config variables are in a try-excpet to prevent errors if a variable is missed in conf.py.

Example site https://slbs.dk/n/ have been updated.

Kwpolska · 2016-11-03T14:23:59Z

v7/navstories/navstories.py

+        for i in self.conf_vars:
+            # Read config variables in a try...except in case a varible is missing
+            try:
+                nav_config[i] = utils.TranslatableSetting(i.lower(), site.config[i], site.config['TRANSLATIONS'])


i.lower() is unnecessary, i will do it too

I didn't know if it should be so - I will remove .lower()

Kwpolska · 2016-11-03T14:29:24Z

How does your plugin behave when /nav/index.html and /nav/last/index.html are in output?

bennyslbs · 2016-11-03T18:20:17Z

I am not sure if I understand your question about /nav/index.html and /nav/last/index.html are in output?
It ignores all files in the output folder.

I will add text similar to this to the README.md:
Navstories seaches only for pages with permalinks listed in NAVSTORIES_PATHS.
It skips the path that matches NAVSTORIES_PATHS. Next level of the permalink is used in the top level menu. If there are further levels it maps to sub-menus.

I don't know if that answers your question?

Kwpolska · 2016-11-03T18:37:34Z

But how does it react to /stories/nav.rst and /stories/nav/last.rst? Where is nav.rst displayed and how?

bennyslbs · 2016-11-04T07:18:36Z

That will not be handled good, it will give two entries in the main menu, one with submenu for all entries in nav/*.rst and one with nav.rst.
As I see it it is not possible to make the mapping well, but one way could be moving the top level nav.rst to first element in submenu, e.g.:
A menu like tihis

  nav
   |- * Top * (nav.rst)
   |- Page 1 (nav/1.rst)
   |- Page 2 (nav/2.rst)

The .lower() isn't needed.

It also handles if a top level page and a submenu, e.g. /nav.rst (top) and /nav/*.rst (sub) It is easier to change how the menu should be generated.

bennyslbs · 2016-11-05T14:04:34Z

Last commit 88ac1a8 changes handling of entries as e.g. nav/*.rst and one with nav.rst.
I have changed it a bit compared to the idea I have listed above:

  nav
   |- Top (nav.rst)
   |- * Page 1 (nav/1.rst)
   |- * Page 2 (nav/2.rst)
   |- * * Page 2a (nav/2/a.rst)

The indention '* ' can be controlled via conf.py.

https://slbs.dk/n have been updated to show this.

bennyslbs · 2016-11-07T16:01:52Z

I don't think I have more changes to the plugin.

navsties skips the lang path specified in TRANSLATIONS for the internal processing (not for the links), but there was an error there. permalink_nolang starts with /pages/... no matter the value of TRANSLATIONS. Version incr t. 1.0.1

Kwpolska

I finally had a while (sorry for taking so long…). It generally looks good, but confusing in some places.

Kwpolska · 2016-11-13T18:55:42Z

v7/navstories/navstories.py

+                pass
+            else:
+                sub = []
+                # Find min/max depth in actual submenu


The min and max functions take iterables and key functions, so you can do this:

min_depth = min(topent[1], key=lambda p: len(p[0]))

Kwpolska · 2016-11-13T18:55:57Z

v7/navstories/navstories.py

+                    prefix = self.navstories_submenu_indention * (len(p[0]) - min_depth)
+                    sub.append(tuple([p[1], prefix + p[2]]))
+                ret.append(tuple([tuple(sub), title]))
+        return tuple(ret)


I thought you were returning a list in this function?

No, it returns a tuple of the same format as NAVIGATION_LINKS (appended to NAVIGATION_LINKS as the last thing that navstories do)

PS. you misspell tuple a lot in your code, so s/tuble/tuple/ please.

Kwpolska · 2016-11-13T18:57:38Z

v7/navstories/navstories.py

+                - Page title
+        Example:
+        [   [   'Menu text for nav',
+                [   [['nav'], '/pages/nav/', 'nav/'],


Perhaps you could make those instances of a NavNode class for better readability (and less random [i][n][d][e][x][i][n][g] everywhere)?

NavNode is a class that you suggests that I create?
I think NavNode should contain:
Member variables:

hierachy

permalink

title
And maybe a constructor method for creating the content based on the page variable (p)

That should do it.

Kwpolska · 2016-11-13T18:58:21Z

v7/navstories/navstories.py

+            # navstories config for lang
+            nav_conf_lang = {}
+            for i in self.conf_vars:
+                 nav_conf_lang[i] = nav_config[i].values[lang]


nav_config[i](lang)

I have seen .values[lang] have been used elsewhere, is the result the same of nav_config[i].values[lang] and nav_config[i](lang)?

.values is internal use and will break if someone assumes the usual behaviour of using default value if language is untranslated to.

Should site.config['NAVIGATION_LINKS'].values[lang] also be changed?
And what about for lang in site.config['NAVIGATION_LINKS'].values:?

for → can’t be changed, not iterable
other instances where there is no iteration: go ahead

Kwpolska · 2016-11-13T19:05:45Z

v7/navstories/navstories.py


+            # Map to tuble
+            new_entries = self.map_to_menu(new)


Wouldn’t an OrderedDict work better for new?

I will let it be an array of navNode's since title=None for entries not in NAVSTORIES_MAPPING, and it would be title there was the natural choise for key in the dict.

Kwpolska · 2016-11-13T19:06:06Z

v7/navstories/navstories.py

+            title = topent[0]
+            if not title:
+                # Default is 1th level of navpath
+                title = topent[1][0][0][0]


Why one, zero, zero, zero? Those indexes are really hard to follow.

That said, you could get rid of at least one by doing

for title, topent_one in entries:

(come up with better name for topent_one), or perhaps OrderedDict (as noted below).

I will take a look on that.

Chris Warrick (Kwpolska) says: .values is internal use and will break if someone assumes the usual behaviour of using default value if language is untranslated to.

NavNode is a new class with parameters for a page (navpath, permalink and title.

bennyslbs · 2016-11-19T07:56:58Z

I have implemented the changes, should I increment the version number again, or is it only when on master it should be incremented?

Kwpolska · 2016-11-19T09:07:01Z

Those version numbers don’t matter.

Kwpolska

Looks pretty good, merging.

Kwpolska · 2016-11-19T17:58:00Z

v7/navstories/navstories.py

+                navpath = permalink_nolang[len(s):].strip('/').split('/')
+                if len(navpath) == 0:
+                    # Should not happen that navpath is empty, but to prevent errors, and inform via a warning
+                    LOGGER.warn("Page with permalink: '%s', title: '%s', not added to menu by navstories.\033[0m" % (p.permalink(lang), p.title(lang)))


Nikola will take care of ending formats itself. (I’ll fix this one.)

Kwpolska · 2016-11-19T17:59:19Z

Thank you for contributing to Nikola! 🎉

navstoris changed to make hierarchical a meny

19cb965

The hierachical menu is based on the structure of the stories (permalink).

Kwpolska reviewed Oct 31, 2016

View reviewed changes

Updated readme after review.

c34532d

bennyslbs added 2 commits November 1, 2016 19:10

Nikola v. 7.8.1: Default location for stories are pages

df8d5d2

Skips initial stories or pages from menu.

navstories: Only add stories specified start of permalink

5667017

permalink of the story shall match one of the entries in NAVSTORIES_PATHS for the given language to be included in the menu.

Kwpolska requested changes Nov 1, 2016

View reviewed changes

Kwpolska changed the title ~~navstoris changed to make hierarchical a meny~~ Make a hierarchical menu in navstories Nov 1, 2016

Restructured and cleanup

8a38615

- Using smarter loops - Minor changes TODO: - p.permalink(lang) ->p.permalink() - Usage of TranslatableSetting

bennyslbs added 2 commits November 2, 2016 21:00

p.permalink(lang) -> p.permalink()

bd62af2

Using TranslatableSettings for NAVSTORIES config.

8361824

bennyslbs added 2 commits November 2, 2016 22:10

Fix permalink(lang) vs. permalink()

2a62874

Create menu structure navpath based on permalink without language prefix, and links in menu including language.

Limited checks during reading config variables

13bd9dd

But read config variables in a try...except in case a varible is missing.

Kwpolska reviewed Nov 3, 2016

View reviewed changes

bennyslbs added 2 commits November 4, 2016 08:39

.lower() removed in TranslatableSetting calls

38ec65a

The .lower() isn't needed.

Restructured to seperate collecting tree structure and generaing menu

88ac1a8

It also handles if a top level page and a submenu, e.g. /nav.rst (top) and /nav/*.rst (sub) It is easier to change how the menu should be generated.

bennyslbs added 2 commits November 7, 2016 16:14

Fix for when TRANSLATION[DEFAULT_LANG] != ''

4e207b5

Updated navstories.plugin for v. 1.0.0.

d94fd4a

Fix with respect to TRANSLATIONS

d3d74eb

navsties skips the lang path specified in TRANSLATIONS for the internal processing (not for the links), but there was an error there. permalink_nolang starts with /pages/... no matter the value of TRANSLATIONS. Version incr t. 1.0.1

Kwpolska requested changes Nov 13, 2016

View reviewed changes

bennyslbs added 4 commits November 16, 2016 20:00

Smarter min_depth and max_depth calculation.

2bcc5b9

Fix comments: tuble->tuple

168a892

.values[lang] -> (lang)

a0cae1c

Chris Warrick (Kwpolska) says: .values is internal use and will break if someone assumes the usual behaviour of using default value if language is untranslated to.

map_to_menu uses list of [toplevel title, NavNode]

e1b0f4c

NavNode is a new class with parameters for a page (navpath, permalink and title.

Kwpolska approved these changes Nov 19, 2016

View reviewed changes

Kwpolska merged commit f17f98f into getnikola:master Nov 19, 2016


		Format of `NAVIGATION_LINKS_POST_NAVSTORIES` is identical to `NAVIGATION_LINKS`.

		Sorting and display names in menu can be controlled for top-level entries via `NAVSTORIES_CFG`.

Navigation Menu

Make a hierarchical menu in navstories #183

Make a hierarchical menu in navstories #183

Conversation

bennyslbs commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennyslbs commented Oct 31, 2016

@Kwpolska commented on this pull request.

Format just as NAVIGATION_LINKS, but content included after navstories entries

bennyslbs commented Oct 31, 2016 via email

Kwpolska commented Oct 31, 2016

bennyslbs commented Oct 31, 2016

bennyslbs commented Nov 1, 2016

Kwpolska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennyslbs Nov 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennyslbs commented Nov 1, 2016

bennyslbs commented Nov 2, 2016

Kwpolska commented Nov 2, 2016 • edited

bennyslbs commented Nov 2, 2016 • edited

bennyslbs commented Nov 2, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kwpolska commented Nov 3, 2016

bennyslbs commented Nov 3, 2016 • edited

Kwpolska commented Nov 3, 2016

bennyslbs commented Nov 4, 2016 • edited

bennyslbs commented Nov 5, 2016 • edited

bennyslbs commented Nov 7, 2016

Kwpolska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennyslbs Nov 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennyslbs commented Nov 19, 2016

Kwpolska commented Nov 19, 2016

Kwpolska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kwpolska commented Nov 19, 2016

bennyslbs Nov 1, 2016 •

edited

Kwpolska commented Nov 2, 2016 •

edited

bennyslbs commented Nov 2, 2016 •

edited

bennyslbs commented Nov 2, 2016 •

edited

bennyslbs commented Nov 3, 2016 •

edited

bennyslbs commented Nov 4, 2016 •

edited

bennyslbs commented Nov 5, 2016 •

edited

bennyslbs Nov 16, 2016 •

edited