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

adding "authoravatar" config parameter #41

Closed
wants to merge 2 commits into from

Conversation

dencold
Copy link

@dencold dencold commented Feb 20, 2016

Hi @vjeantet, merci beaucoup for creating this hugo theme! I started porting the original from Ghost and then I found hugo's theme repository and saw that you had already done the hard work for me ;)

I noticed that currently the logo variable on config.toml represents two states:

  • the logo at the top-left of the website
  • the picture for the post’s author

If you want to have a picture of the author that is not the same as the logo, it's not currently supported.

This PR uncouples this relationship and creates an explicit configuration for each. logo now only represents the image at the left-hand corner of the website and the new authoravatar represents
the image of the author.

I've also updated the README.md so that the documentation reflects these changes.

Let me know if this makes sense and is good to merge into your origin.

Again, thanks so much for the work here!!
--Dennis

currently, the “logo” variable represents *two* states:

- the logo at the top-left of the website
- the picture for the post’s author

this commit uncouples this relationship and creates an explicit
configuration for each. “logo” now only represents the image at the
left-hand corner of the website and the new “authoravatar” represents
the image of the author.
@Jean85
Copy link

Jean85 commented Mar 6, 2016

Would love to have this!

Wait, this is already implemented with the thumbnail parameter in the authors sections!
Also, this PR doesn't work with multiple authors.

👎

@dencold
Copy link
Author

dencold commented Mar 7, 2016

Hi @Jean85, thanks for commenting on the PR! I saw the thumbnail parameter when I was first getting started with @vjeantet's theme. However, it never worked for me, even after setting the appropriate config in data/authors/*.yaml.

I've taken a more thorough look into the source code now and I think the problem is currently in 3 places:

  1. Thumbnail logic is not included anywhere on the single.html layout, the problem is with these lines. You have referenced this yourself in Author thumbnail is not used in the post footer #42.
  2. The code that is used to determine if a thumbnail is used on the list.html layout only tries to match on Params.author which means every post has to include an author section in its front matter. If you forget to do so, the theme reverts back to the logo.
  3. The current version of the example.yaml file makes no references to a thumbnail field, which makes it confusing for people new to the theme.

My use case is a single-author blog. My apologies that my PR, as it currently stands, does not help address the issue for multiple authors. I am going to work on this now and should have a fix that will work both for me (single-author) and you (multi-author) by tomorrow.

Thanks again for bringing this to my attention.
--Dennis

@Jean85
Copy link

Jean85 commented Mar 7, 2016

You're welcome! Thanks to you too!!
I was also looking into using an absolute URL as a thumbnail (I would like to use Gravatars) but I'm not sure if I will need an other parameter..

Do you think is feasible to use relative (in-Hugo) and absolute (external) URL for thumbnails inside the same parameter? Do we have a way to distinguish them from inside the template?

@dencold
Copy link
Author

dencold commented Mar 7, 2016

Currently, only internal thumbnails are supported. The code always uses your site's BaseURL to set background-image.

It's possible to change this logic to accept external resources, but for now I am going to focus on support for multiple authors with internal thumbnails. We can open another PR down the road which addresses external support.

@Jean85
Copy link

Jean85 commented Mar 7, 2016

Mmmh... Maybe just removing BaseURL is enough? If you specify your thumbnails starting from the root, you could use it both ways!

@dencold
Copy link
Author

dencold commented Mar 7, 2016

@Jean85 yep, that's exactly what I was thinking. but I want to keep focused and not change this PR's scope. There are multiple places in the theme that will need to change in order to support external image references.

- no longer dependent on Site.Data, we now use Site.Author as the
  canonical source for Author metadata
- correctly checking for author setting on the post front-matter *or*
  the site's config file
- thumbnails now work on both list and single layouts
@dencold
Copy link
Author

dencold commented Mar 8, 2016

@Jean85 I think this should all be addressed now with the latest commit 8206674. There is a major change in the way authors are configured, however. Read on for the details.

Old Way

There were two ways to set up metadata for author. Either you added the author, authorlocation, authorwebsite, etc. to the .Site.Params section of config.yaml or you created a separate file in the data/authors subdirectory of your site folder.

This was a bit confusing and breaks the DRY principle.

New Way

Hugo already provides a canonical source for author metadata, the Site.Author section. I've changed the code to directly reference it. Instead of having a separate author.yaml file in the data directory, you can now just add the following to your config.yaml:

author:
  david:
    name: "David Hasselhoff"
    bio: "Don't Hassle the Hoff"
    location: "Baltimore, MD"
    thumbnail: "images/avatar.jpg"

You can add multiple authors simply by appending them under their own key in the author section. For example:

author:
  david:
    name: "David Hasselhoff"
    bio: "Don't Hassle the Hoff"
    location: "Baltimore, MD"
    thumbnail: "images/avatar.jpg"
  pamela:
    name: "Pamela Anderson"
    bio: "Little known fact, I am vegan"
    location: "Canada"
    thumbnail: "images/pamela.jpg"

You can then have either a site-wide single author or an author per-post. As long as the author tag matches one of the keys in the Site.Author section, you should be good to go. Let me know if that makes sense.

I've also created my own version of this repository which contains all of the fixes here, as well as more. You can get that here:
https://github.com/dencold/hasper

The README also tries to explain more there.

@Jean85
Copy link

Jean85 commented Mar 8, 2016

I'm not sure about breaking the DRY... The author specified in the config is the "default" author, that can be (optionally) overwritten in the front matter. Why this should be an issue?

@dencold
Copy link
Author

dencold commented Mar 8, 2016

Why have two places that authors can be configured? Additionally the .Site.Data.Author is non-standard in the Hugo ecosystem and is not obvious to someone looking at the config.yaml. You can't tell what authors are configured for the blog without remembering to check all the files in your site's data/authors directory.

I prefer solutions that are self-documenting and straightforward.

In any case. This all appears to be moot. The author is not joining the conversation and the PR has been outstanding for 3 weeks. I am switching focus over to my own repository to move things forward. You are welcome to continue the discussion there, if it suits you:
https://github.com/dencold/hasper

@dencold dencold closed this Mar 8, 2016
@Jean85
Copy link

Jean85 commented Mar 8, 2016

I'm not very familiar with Hugo, I didn't know that this wasn't a standard approach. Thanks, I'll look into your fork.

@dencold
Copy link
Author

dencold commented Mar 8, 2016

Ahhh, gotcha. Yeah, you can see all of the default variables Hugo supports on their documentation page:
http://gohugo.io/templates/variables/#site-variables

I will be moving things forward on my fork.

@dencold
Copy link
Author

dencold commented Mar 9, 2016

@Jean85 just a last heads up, I've also implemented the changes needed for absolute images, which was another request you had. Here is the commit for this change in hasper:
dencold/hasper@06afad2

@Jean85
Copy link

Jean85 commented Mar 9, 2016

Woha great! Thanks!

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