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

@Kwpolska Kwpolska commented Dec 18, 2016

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

@felixfontein felixfontein Dec 18, 2016

Why not precompute a LUT in postprocess_posts_per_classification instead of recomputing this every time this function is called?

Copy link
Member Author

@Kwpolska Kwpolska Dec 18, 2016

Do you mean a lookup table for flat_samelevel, or for idx too?

Copy link
Contributor

@felixfontein felixfontein Dec 18, 2016

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

@felixfontein felixfontein Dec 18, 2016

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

@Kwpolska Kwpolska Dec 18, 2016

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

@felixfontein felixfontein Dec 18, 2016

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

@felixfontein felixfontein Dec 18, 2016

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

@felixfontein felixfontein Dec 18, 2016

Your calls to self.site.link (this one and the two below) must specify the taxonomy name (here: archive).

Kwpolska added 2 commits Dec 18, 2016
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska
Copy link
Member Author

@Kwpolska Kwpolska commented Dec 18, 2016

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

@felixfontein felixfontein commented Dec 18, 2016

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

@Kwpolska
Copy link
Member Author

@Kwpolska Kwpolska commented Dec 19, 2016

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

@felixfontein felixfontein commented Dec 23, 2016

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

@Kwpolska
Copy link
Member Author

@Kwpolska Kwpolska commented Dec 23, 2016

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

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Dec 23, 2016

My review was (mostly) for the code, not for the theme support :)

Kwpolska added 2 commits Dec 23, 2016
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@Kwpolska
Copy link
Member Author

@Kwpolska Kwpolska commented Dec 24, 2016

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

@felixfontein felixfontein commented Dec 25, 2016

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

@Kwpolska Kwpolska commented Dec 26, 2016

Sure, do you want to implement that?

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Dec 26, 2016

That should do the job.

@Kwpolska Kwpolska merged commit 375b634 into master Dec 27, 2016
5 checks passed
@Kwpolska Kwpolska deleted the archive-navigation branch Dec 27, 2016
@Kwpolska
Copy link
Member Author

@Kwpolska Kwpolska commented Dec 27, 2016

Thanks for the help!

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Dec 27, 2016

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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants