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

Make a hierarchical menu in navstories #183

Merged
merged 18 commits into from Nov 19, 2016
Merged

Conversation

@bennyslbs
Copy link
Contributor

@bennyslbs bennyslbs commented Oct 31, 2016

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

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

This comment has been minimized.

@Kwpolska

Kwpolska Oct 31, 2016
Member

yes or True

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

This comment has been minimized.

@Kwpolska

Kwpolska Oct 31, 2016
Member

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


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

This comment has been minimized.

@Kwpolska

Kwpolska Oct 31, 2016
Member

no example for this

@bennyslbs
Copy link
Contributor Author

@bennyslbs bennyslbs commented Oct 31, 2016

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

@bennyslbs bennyslbs commented Oct 31, 2016

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Oct 31, 2016

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

@bennyslbs bennyslbs commented Oct 31, 2016

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

bennyslbs added 2 commits Nov 1, 2016
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
Copy link
Contributor Author

@bennyslbs bennyslbs commented Nov 1, 2016

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.

Copy link
Member

@Kwpolska Kwpolska left a comment

How does the resulting menu look for a sample site?

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 1, 2016
Member

one “for”

This comment has been minimized.

@bennyslbs

bennyslbs Nov 1, 2016
Author Contributor

I'l fix it


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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 1, 2016
Member

startswith can take a tuple.

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

This comment has been minimized.

@bennyslbs

bennyslbs Nov 1, 2016
Author Contributor

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.

This comment has been minimized.

@Kwpolska

Kwpolska Nov 1, 2016
Member

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')}

This comment has been minimized.

@bennyslbs

bennyslbs Nov 1, 2016
Author Contributor

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 2, 2016
Member

Sure.

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 1, 2016
Member

What do you mean here with the comment?

This comment has been minimized.

@bennyslbs

bennyslbs Nov 1, 2016
Author Contributor

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

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 comment has been minimized.

@Kwpolska

Kwpolska Nov 1, 2016
Member

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

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

This comment has been minimized.

@bennyslbs

bennyslbs Nov 1, 2016
Author Contributor

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.

This comment has been minimized.

@Kwpolska

Kwpolska Nov 1, 2016
Member

  1. You could use TranslatableSetting on all your setting to make things less of a mess.
  2. Do the rename. And add some comments to explain your range(len()) thing.
  3. 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.

This comment has been minimized.

@bennyslbs

bennyslbs Nov 1, 2016
Author Contributor

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/

This comment has been minimized.

@Kwpolska

Kwpolska Nov 2, 2016
Member

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

@bennyslbs
Copy link
Contributor Author

@bennyslbs bennyslbs commented Nov 1, 2016

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

@Kwpolska Kwpolska changed the title navstoris changed to make hierarchical a meny Make a hierarchical menu in navstories Nov 1, 2016
- Using smarter loops
- Minor changes

TODO:
- p.permalink(lang) ->p.permalink()
- Usage of TranslatableSetting
@bennyslbs
Copy link
Contributor Author

@bennyslbs bennyslbs commented Nov 2, 2016

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

@Kwpolska Kwpolska commented Nov 2, 2016

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.

@Kwpolska

This comment has been minimized.

Copy link
Member

@Kwpolska Kwpolska commented on bd62af2 Nov 2, 2016

undo this!

@bennyslbs
Copy link
Contributor Author

@bennyslbs bennyslbs commented Nov 2, 2016

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.

bennyslbs added 2 commits Nov 2, 2016
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
Copy link
Contributor Author

@bennyslbs bennyslbs commented Nov 2, 2016

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.

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'])

This comment has been minimized.

@Kwpolska

Kwpolska Nov 3, 2016
Member

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

This comment has been minimized.

@bennyslbs

bennyslbs Nov 3, 2016
Author Contributor

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

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Nov 3, 2016

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

@bennyslbs
Copy link
Contributor Author

@bennyslbs bennyslbs commented Nov 3, 2016

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

@Kwpolska Kwpolska commented Nov 3, 2016

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

@bennyslbs
Copy link
Contributor Author

@bennyslbs bennyslbs commented Nov 4, 2016

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)
bennyslbs added 2 commits Nov 4, 2016
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
Copy link
Contributor Author

@bennyslbs bennyslbs commented Nov 5, 2016

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

@bennyslbs bennyslbs commented Nov 7, 2016

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

@Kwpolska Kwpolska left a comment

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

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 13, 2016
Member

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

This comment has been minimized.

@bennyslbs

bennyslbs Nov 15, 2016
Author Contributor

Done :-)

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)

This comment has been minimized.

@Kwpolska

Kwpolska Nov 13, 2016
Member

I thought you were returning a list in this function?

This comment has been minimized.

@bennyslbs

bennyslbs Nov 15, 2016
Author Contributor

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 16, 2016
Member

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

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 13, 2016
Member

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

This comment has been minimized.

@bennyslbs

bennyslbs Nov 15, 2016
Author Contributor

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)

This comment has been minimized.

@Kwpolska

Kwpolska Nov 16, 2016
Member

That should do it.

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 13, 2016
Member

nav_config[i](lang)

This comment has been minimized.

@bennyslbs

bennyslbs Nov 15, 2016
Author Contributor

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 16, 2016
Member

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

This comment has been minimized.

@bennyslbs

bennyslbs Nov 16, 2016
Author Contributor

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 16, 2016
Member

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


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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 13, 2016
Member

Wouldn’t an OrderedDict work better for new?

This comment has been minimized.

@bennyslbs

bennyslbs Nov 16, 2016
Author Contributor

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.

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 13, 2016
Member

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

This comment has been minimized.

@bennyslbs

bennyslbs Nov 15, 2016
Author Contributor

I will take a look on that.

bennyslbs added 4 commits Nov 16, 2016
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
Copy link
Contributor Author

@bennyslbs bennyslbs commented Nov 19, 2016

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

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Nov 19, 2016

Those version numbers don’t matter.

Copy link
Member

@Kwpolska Kwpolska left a comment

Looks pretty good, merging.

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

This comment has been minimized.

@Kwpolska

Kwpolska Nov 19, 2016
Member

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

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Nov 19, 2016

Thank you for contributing to Nikola! 🎉

@Kwpolska Kwpolska merged commit f17f98f into getnikola:master Nov 19, 2016
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants