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

yosevu · 2020-09-26T23:15:03Z

Add site-title site-author, and site-desc to layouts public-api
Update default layout
Update head partial
Update meta data table docs
Make meta charset casing consistent with other attributes

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.

teesloane

Thanks for the PR @yosevu. I made a couple small suggestions. I should be able to download the artifact of the Firn CI build and test it whenever you think it's ready (you might be able to download it too if Graal is causing issues.)

teesloane · 2020-09-28T10:39:30Z

clojure/src/firn/layout.clj

@@ -160,6 +160,9 @@
     :site-map      (config :site-map)
     :site-links    (config :site-links)
     :site-logs     (config :site-logs)
+     :site-title    (config :site-title)
+     :site-author   (config :site-author)
+     :site-desc     (config :site-desc)


This is great! Do you mind updating the docs table to match the updated layout vals?

FYI, the templates for the docs site can also be updated to include a title, author, and description.

teesloane · 2020-09-28T10:40:23Z

clojure/resources/firn/_firn_starter/layouts/default.clj

@@ -1,8 +1,8 @@
 (defn default
-  [{:keys [render partials build-url]}]
+  [{:keys [render partials build-url site-title site-author site-desc]}]


Suggested change

[{:keys [render partials build-url site-title site-author site-desc]}]

[{:keys [render partials build-url site-title site-author site-desc] :as config}]

We should probably just keep the map value around so that we can pass it to the head partial`, then update the head partial to deconstruct what it needs from the config map.

teesloane · 2020-09-28T10:40:49Z

clojure/resources/firn/_firn_starter/layouts/default.clj

  (let [{:keys [head]} partials]
    [:html
-     (head build-url)
+     (head build-url site-title site-author site-desc)


Suggested change

(head build-url site-title site-author site-desc)

(head config)

teesloane · 2020-09-28T10:41:23Z

clojure/resources/firn/_firn_starter/partials/head.clj

@@ -1,7 +1,10 @@
 (defn head
-  [build-url]
+  [build-url site-title site-author site-desc]


Suggested change

[build-url site-title site-author site-desc]

[{:keys [build-url site-title site-author site-desc]}]

yosevu · 2020-09-30T02:58:57Z

A couple questions @teesloane:

Are only required config properties being checked in layout_test.clj?
Are there any guidelines for commit style? It looks like you are using something similar to conventional commits.

teesloane · 2020-09-30T09:57:19Z

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.

teesloane

I forgot that values from config.edn are loaded into the submap user-config and so need to be accessed at (-> config :user-config :the-value)

clojure/src/firn/layout.clj

yosevu · 2020-10-01T03:30:27Z

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

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

yosevu · 2020-10-01T03:30:39Z

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:

teesloane · 2020-10-01T10:18:50Z

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.

- Add site title, author, and description to layouts public-api for SEO and accessibility. - Update default layout - Update head partial - Make meta charset casing consistent with other attributes - Update metadata table - Add site-author, site-desc, and site-title - Add site-url - Add firn-tags data type - Alphabetize and format

yosevu · 2020-10-01T12:10:58Z

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: #42.

teesloane · 2020-10-01T12:24:14Z

Thanks! Merged both!

- Add site title, author, and description to layouts public-api for SEO and accessibility. - Update default layout - Update head partial - Make meta charset casing consistent with other attributes - Update metadata table - Add site-author, site-desc, and site-title - Add site-url - Add firn-tags data type - Alphabetize and format

teesloane requested changes Sep 28, 2020

View reviewed changes

yosevu force-pushed the fix--add-site-title-to-public-api-and-default-layout branch from db669a6 to 3fa6dd8 Compare September 30, 2020 02:44

teesloane requested changes Sep 30, 2020

View reviewed changes

clojure/src/firn/layout.clj Outdated Show resolved Hide resolved

clojure/src/firn/layout.clj Outdated Show resolved Hide resolved

clojure/src/firn/layout.clj Outdated Show resolved Hide resolved

yosevu marked this pull request as ready for review October 1, 2020 03:34

yosevu requested a review from teesloane October 1, 2020 03:38

yosevu force-pushed the fix--add-site-title-to-public-api-and-default-layout branch from 2e3c15e to 4468f43 Compare October 1, 2020 11:48

teesloane merged commit a2a6b40 into teesloane:master Oct 1, 2020

yosevu deleted the fix--add-site-title-to-public-api-and-default-layout branch October 1, 2020 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yosevu commented Sep 26, 2020 •

edited

Loading

teesloane left a comment

teesloane Sep 28, 2020

yosevu Sep 30, 2020

yosevu Sep 30, 2020

teesloane Sep 28, 2020

yosevu Sep 30, 2020

teesloane Sep 28, 2020

teesloane Sep 28, 2020

yosevu commented Sep 30, 2020

teesloane commented Sep 30, 2020

teesloane left a comment

yosevu commented Oct 1, 2020 •

edited

Loading

yosevu commented Oct 1, 2020 •

edited

Loading

teesloane commented Oct 1, 2020

yosevu commented Oct 1, 2020

teesloane commented Oct 1, 2020

	[{:keys [render partials build-url site-title site-author site-desc]}]
	[{:keys [render partials build-url site-title site-author site-desc] :as config}]

	(head build-url site-title site-author site-desc)
	(head config)

	[build-url site-title site-author site-desc]
	[{:keys [build-url site-title site-author site-desc]}]

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

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

Conversation

yosevu commented Sep 26, 2020 • edited Loading

teesloane left a comment

Choose a reason for hiding this comment

teesloane Sep 28, 2020

Choose a reason for hiding this comment

yosevu Sep 30, 2020

Choose a reason for hiding this comment

yosevu Sep 30, 2020

Choose a reason for hiding this comment

teesloane Sep 28, 2020

Choose a reason for hiding this comment

yosevu Sep 30, 2020

Choose a reason for hiding this comment

teesloane Sep 28, 2020

Choose a reason for hiding this comment

teesloane Sep 28, 2020

Choose a reason for hiding this comment

yosevu commented Sep 30, 2020

teesloane commented Sep 30, 2020

teesloane left a comment

Choose a reason for hiding this comment

yosevu commented Oct 1, 2020 • edited Loading

yosevu commented Oct 1, 2020 • edited Loading

teesloane commented Oct 1, 2020

yosevu commented Oct 1, 2020

teesloane commented Oct 1, 2020

yosevu commented Sep 26, 2020 •

edited

Loading

yosevu commented Oct 1, 2020 •

edited

Loading

yosevu commented Oct 1, 2020 •

edited

Loading