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

Archive navigation #2599

Merged
merged 10 commits into from Dec 27, 2016
Merged

Archive navigation #2599

merged 10 commits into from Dec 27, 2016

Conversation

Kwpolska
Copy link
Member

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)

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

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?

Copy link
Member Author

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?

Copy link
Contributor

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

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.

Copy link
Member Author

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.

Copy link
Contributor

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

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

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>
@Kwpolska
Copy link
Member Author

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.

@felixfontein
Copy link
Contributor

list_post.tmpl for post lists, archiveindex.tmpl for archives, and list.tmpl for list of years or months.

@Kwpolska
Copy link
Member Author

Why are there so many?! I’ll have to move some things then, guess I’ll go back to depending on pagekind.

Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@felixfontein
Copy link
Contributor

I haven't tested it, but I think it looks good.

@Kwpolska
Copy link
Member Author

I didn’t finish the theme support yet, not merging.

@felixfontein
Copy link
Contributor

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>
@Kwpolska
Copy link
Member Author

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)

@felixfontein
Copy link
Contributor

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

@Kwpolska
Copy link
Member Author

Sure, do you want to implement that?

@felixfontein
Copy link
Contributor

That should do the job.

@Kwpolska Kwpolska merged commit 375b634 into master Dec 27, 2016
@Kwpolska Kwpolska deleted the archive-navigation branch December 27, 2016 09:15
@Kwpolska
Copy link
Member Author

Thanks for the help!

@felixfontein
Copy link
Contributor

You're welcome. I was just about to write that I tested this (with my own themes) and it works great!

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

2 participants