gohugoio / hugoThemes

A curated directory of Hugo themes
https://themes.gohugo.io/
MIT License
1.77k stars 244 forks source link

New Theme: 'Lapis' #638

Closed jdahm closed 5 years ago

jdahm commented 5 years ago

I'd like to submit a Hugo theme based on https://github.com/sergiokopplin/indigo for Jekyll. I use it for a simple landing page on my site https://github.com/jdahm/personal-site. This is my first theme submission, and I'm fairly new to Hugo, so I'm open to feedback about best practices and what could be improved. Thanks!

I added default options for hugoBasicExample to data/default.toml. Is this the best place for those defaults?

There are a few directories under the hugoBasicExample content/ directory that I added to the default navigation list, but most of these don't seem to link to anything. What is the intentions behind including those content files?

Link to my theme repository: https://github.com/jdahm/lapis

I made sure that...

digitalcraftsman commented 5 years ago

Hello @jdahm,

welcome to the Hugo community and thank you for your them submission 👍

I added default options for hugoBasicExample to data/default.toml. Is this the best place for those defaults?

Your default values look very much like a config file. If you want to use a custom config file for the demo of your theme, you can add a new one at exampleSite/config.toml.

For the menu I would suggest to use Hugo's built-in functionality to create menus.

There are a few directories under the hugoBasicExample content/ directory that I added to the default navigation list, but most of these don't seem to link to anything. What is the intentions behind including those content files?

tl;dr don't worry about it. They've been added for specific themes that require special content files. Just remove the "Sketch" and "Portfolio" link and we are good to go.

Long version: Some files / directories have been added to be compatible with themes that have a hard-coded content directory (e.g. posts). The sketch directory has been added for a theme that doesn't need to be whitelisted for the use of custom content files but wanted to include this interactive demo. For more information see https://github.com/gohugoio/hugoBasicExample/pull/42.

Otherwise you're theme looks good as well as the code in the template files.

jdahm commented 5 years ago

Thanks @digitalcraftsman!

Your default values look very much like a config file. If you want to use a custom config file for the demo of your theme, you can add a new one at exampleSite/config.toml.

My idea when adding the default values was to avoid including a full exampleSite and instead use hugoBasicExample. I am not opposed to a config file, but If I add those at exampleSite/config.toml then I have to include the whole site (mostly adding example blog posts), correct?

digitalcraftsman commented 5 years ago

I thought again about a custom config file and came to the conclusion that most users will be fine if they add a config file if needed and the default values should cover most things. They can however be minor drawbacks, e.g. if a user wants to add a content file to a menu via its front matter due to the way the navigation is implemented.

Nonetheless, I'm fine with your current approach. Having only a config file in exampleSite still works because for everything else the build script falls back to the hugoBasicExample.

The sketch and portfolio link in the menu however will still not work. If you remove them to avoid 404 errors, we're ready to Lapis.

jdahm commented 5 years ago

I switched to exampleSite and Hugo menus. It should be ready to go. Could you take a look?

digitalcraftsman commented 5 years ago

Hey Johann,

you changes look good. While looking at your theme again I noticed that you specified a profile picture for the blog owner but the file isn't present in the repo. You can you add an example picture, e.g. from www.uifaces.co

In the header.html partial your are displaying the title of the current page but you always link the homepage. On some pages this can be a bit misleading, e.g. if users are browsing the tags at /tags. When I read "tags" as title I wouldn't expect that it links the homepage.

When I'm on the about page there's no link back to the homepage. Would you consider to add a "Home" link (or something similar) to the menu?

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. Feel free to ask questions. We're glad to help. This issue will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

jdahm commented 5 years ago

I'm still planning to work on this. I hope to have a new version this weekend.

jdahm commented 5 years ago

Hey @digitalcraftsman,

While looking at your theme again I noticed that you specified a profile picture for the blog owner but the file isn't present in the repo. You can you add an example picture, e.g. from www.uifaces.co

Thanks for catching that! I added one from there.

In the header.html partial your are displaying the title of the current page but you always link the homepage. On some pages this can be a bit misleading, e.g. if users are browsing the tags at /tags. When I read "tags" as title I wouldn't expect that it links the homepage.

When I'm on the about page there's no link back to the homepage. Would you consider to add a "Home" link (or something similar) to the menu?

Good idea. I switched back to adding a "Home" as Indigo does.

digitalcraftsman commented 5 years ago

Dear Johann,

excuse the delays in the review process of your theme. Right now, I'm in the final phase of finishing my bachelor thesis. Hence I'm want to ask for a little patience.

Thank you.

jdahm commented 5 years ago

@digitalcraftsman no need to apologize. Your work is much more important. This can happen any time later.

onedrawingperday commented 5 years ago

Hello @jdahm

I will be filling in for the rest of this review.

First of all your theme renders fine when I execute the Build Script, I have only noticed the following issues:

  1. The HTML being used here is deprecated please remove the xlink:href attribute because otherwise the HTML output will be invalid.

  2. On permalink pages of the various blog posts I have noticed that the title of each page is not rendered on the page. Is this an intended design choice?

jdahm commented 5 years ago

@onedrawingperday Thanks for reviewing the theme! As I mentioned to @digitalcraftsman, this is my first theme and I'm more of a c++ developer by trade, so I really appreciate your help. My apologies for the delay, I do plan to get to this soon, but urgent projects at work have priority.

onedrawingperday commented 5 years ago

@jdahm No worries. Take your time.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. Feel free to ask questions. We're glad to help. This issue will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.