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

Add option to hide ReadingTime, simplify duplicate code #15

Merged
merged 2 commits into from May 26, 2016

Conversation

Projects
None yet
2 participants
@Erethon
Contributor

Erethon commented May 25, 2016

Hello, thank you for porting this theme to Hugo!

This patch set:

  • Moves duplicate code into a partial template. I'm not familiar with Go
    templating, so I used Scratch, there
    might be a better way to do this.
  • Formats the code a bit to be more readable.
  • Adds an option to hide the ReadingTime label on posts.
  • As a side effect, removes an orphan - character when disqus is disabled
    and a post has no categories.
@parsiya

This comment has been minimized.

Owner

parsiya commented May 26, 2016

This is great, thanks a lot.

Hugo has a function called IsHome that returns true for index (I think it's undocumented but works in v0.15 which I use). We can use that instead of the scratch variable.

{{ if $.IsHome }}this is the index{{ end }}

So line 5 in post_header.html which is:

{{ if (.Scratch.Get "IsStandAlone") }} {{ .Title }} {{ else }} <a href="{{ .Permalink }}">{{ .Title }}</a> {{ end }}

Can be re-written as:

{{ if not $.IsHome }} {{ .Title }} {{ else }} <a href="{{ .Permalink }}">{{ .Title }}</a> {{ end }}

And then the scratch variable can be omitted.

Pardon my ignorance, how do we proceed? Do I approve your merge request and then make the change? What is the etiquette in this case? (not a dev) :)

Erethon added some commits May 25, 2016

Create a partial template for the header of posts
* Move duplicate code that handles the header/title of a post in a
partial template.
* Format code a bit.

@Erethon Erethon force-pushed the Erethon:master branch from e42f9e4 to 584ddb0 May 26, 2016

@Erethon

This comment has been minimized.

Contributor

Erethon commented May 26, 2016

Hugo has a function called IsHome that returns true for index (I think it's undocumented but works in v0.15 which I use). We can use that instead of the scratch variable.

I see, lets use that then!

Can be re-written as:

{{ if not $.IsHome }} {{ .Title }} {{ else }} <a href="{{ .Permalink }}">{{ .Title }}</a> {{ end}}

I've re-written this the other way around to be more readable, aka:
{{ if .IsHome }} <a href="{{ .Permalink }}">{{ .Title }}</a> {{ else }} {{ .Title }} {{ end }}

Pardon my ignorance, how do we proceed? Do I approve your merge request and then make the change? What is the etiquette in this case? (not a dev) :)

There are a couple of ways to do this:

  1. I could create a new commit with your proposed changes, push it to my repo and GitHub would pick it up and add it in this PR. In this case we would have three commits and the last one would essentially alter the changes of the first commit.
  2. I could rebase/edit my initial commits with your changes, force push in my repo and GitHub would once agian pick it up and update this PR. In this case, there are only two commits and the git history is much cleaner imho.

I've gone with option (2), feel free to review the commits again and merge this if everything looks ok to you.

@parsiya parsiya merged commit 22fb936 into parsiya:master May 26, 2016

@parsiya

This comment has been minimized.

Owner

parsiya commented May 26, 2016

Great, thanks a lot for helping with the theme and teaching me git 👍

@parsiya parsiya added the bug label May 26, 2016

@parsiya

This comment has been minimized.

Owner

parsiya commented May 27, 2016

My suggestion has killed the permalink on the titles in home. When we are calling the partial, we are inside a page (we are in the paginator range) because we are actually getting content from a page and the partial gets the page variable passed to it (via the . after the partial name), see Variable scoping. As such .IsHome does not work inside the partial because it is pointing to the page.

I should have detected it when I realized that .IsHome does not work inside the paginator loop and only works via $.IsHome which points to the index page.

I will make changes according to your version of the scratch variables if that is Ok with you.

Again thanks for your help :)

@Erethon

This comment has been minimized.

Contributor

Erethon commented May 27, 2016

I will make changes according to your version of the scratch variables if that is Ok with you.

Sure, feel free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment