teesloane / firn

Org Mode Static Site Generator
Eclipse Public License 1.0
325 stars 24 forks source link

Fix: add site title, author, and description to layouts #41

Closed yosevu closed 4 years ago

yosevu commented 4 years ago

Hey @teesloane, I noticed that there is no site title or meta author and description. Can they be added for SEO and accessibility?

https://developer.mozilla.org/en-US/docs/Learn/HTML/Introduction_to_HTML/The_head_metadata_in_HTML#Adding_an_author_and_description

I haven't been able to get my development env working yet with GraalVM to test these changes, but I wanted to check first before I go any further in case you had other plans.

yosevu commented 4 years ago

A couple questions @teesloane:

teesloane commented 4 years ago

A couple questions @teesloane:

* Are only required config properties being checked in [layout_test.clj](https://github.com/theiceshelf/firn/blob/master/clojure/test/firn/layout_test.clj#L13)?

Nah, I kind of forgot about that test. It's a bit flimsy. I think I made it to make sure that config property names aren't being overwritten or removed. You can either add the new keys or leave it for a better test in the future.

* Are there any guidelines for commit style? It looks like you are using something similar to [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/).

I've actually just borrowed my commit leaders from here — and didn't know about conventional commits. Going forward (if not now, then at least at first major release) I think this might be a good choice.

yosevu commented 4 years ago

Updated to correctly get user config values and I'm able to verify it now:

Screen Shot 2020-09-30 at 11 18 06 PM

I can clean up and squash my commits if everything else looks good @teesloane.

yosevu commented 4 years ago

I did run into an issue with a circular dependency in org.clj. This looks like a bug to me, but I wanted to check first to see if you are running into it too @teesloane:

Screen Shot 2020-09-30 at 11 13 31 PM
teesloane commented 4 years ago

Looks good! As for the circular dependency - you are requiring firn.org from within firn.org on line 7. I guess it can still compile in that case? Either way, better to remove it. It looks like I can squash and merge into master from Github, but feel free to rebase and clean commits as you like.

yosevu commented 4 years ago

I squashed my commits. The circular dependency comment was unrelated to this PR. I thought it would be better to create a separate one to remove it: https://github.com/theiceshelf/firn/pull/42.

teesloane commented 4 years ago

Thanks! Merged both!