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
Archive navigation #2599
Archive navigation #2599
Conversation
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
flat_samelevel = [node.classification_name for node in flat_hierarchy if len(node.classification_path) == nodelevel] | ||
# We need to sort it. Natsort means it’s year 10000 compatible! | ||
flat_samelevel = natsort.natsorted(flat_samelevel, alg=natsort.ns.F | natsort.ns.IC) | ||
idx = flat_samelevel.find(classification) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not precompute a LUT in postprocess_posts_per_classification
instead of recomputing this every time this function is called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean a lookup table for flat_samelevel
, or for idx
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both of them. You can also make a lookup for the resulting nodes / classifications if you want.
previdx, nextidx = idx - 1, idx + 1 | ||
# If the previous index is -1, or the next index is 1, the previous/next archive does not exist. | ||
context["previous_archive"] = self.site.link(flat_samelevel[previdx], lang) if previdx != -1 else None | ||
context["next_archive"] = self.site.link(flat_samelevel[nextidx], lang) if nextidx != -1 else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would make sense to also insert the names of the archives. Links for the Atom feeds (see da2x' comments in #1639) would be nice, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those links do not sound anywhere near useful to me. If someone really needs an Atom link to the archives for a previous level (why?), they can just click the link they want and then the Atom link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understood @da2x correctly, these links are to be inserted in the Atom feed for this archive itself. But I don't know enough about Atom feeds to be sure that's the case.
context["up_level"] = None | ||
|
||
flat_hierarchy = self.site.flat_hierarchy_per_classification['archive'][lang] | ||
nodelevel = len(hierarchy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also insert nodelevel
into the context so that link texts (next year/month/day) can be chosen appropriately without unnecessary complicated code (like manually counting the number of slashes in the classification etc.).
if hierarchy: | ||
# Up level link makes sense only if this is not the top-level | ||
# page (hierarchy is empty) | ||
context["up_level"] = self.site.link('/'.join(hierarchy[:-1]), lang) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your calls to self.site.link
(this one and the two below) must specify the taxonomy name (here: archive
).
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
I tried adding some support in templates, but for some reason, it doesn’t work in the monthly archives (2012/03 for example). What template do they use? It looks really strange, because the right variables are in the context. |
|
Why are there so many?! I’ll have to move some things then, guess I’ll go back to depending on |
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
I haven't tested it, but I think it looks good. |
I didn’t finish the theme support yet, not merging. |
My review was (mostly) for the code, not for the theme support :) |
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Alright, theming is done, could use a review of that and some testing (although I did my best to make it work, maybe there’s some edge case I didn’t notice) |
How about also inserting the names of the linked to archives into the context? That would give themes more flexibility (i.e. incorporating the archive's name instead of just naming the links next/previous/up). |
Sure, do you want to implement that? |
That should do the job. |
Thanks for the help! |
You're welcome. I was just about to write that I tested this (with my own themes) and it works great! |
This is a simple solution for #1639, done in 26 readable lines. This is not finished, template stuff and perhaps friendly names need to be worked in.
cc @felixfontein, @magmax
(PS. I also did some minor cleanups re TreeNodes on master in c79164b, 6027bf5, 41162b2)