cjtheham / hugo-theme-readable

Hugo theme built using readable.css from Freedom to Write
The Unlicense
6 stars 4 forks source link

feat: add params.navbar_style to config to select a style for the navbar #14

Closed rluetzner closed 1 year ago

rluetzner commented 1 year ago

This is a new feature in readable.css 1.1.0. The new version of readable.css defaults to a navbar without animations, but to stay compatible with older releases of this theme, we default to "classy" instead which is the animated navbar.

rluetzner commented 1 year ago

This builds on top of #13 and should only be merged afterwards. It'll probably cause a merge conflict once #13 is merged. The conflict should be easy enough to resolve, but I can also take care of that when the time comes. :slightly_smiling_face:

rluetzner commented 1 year ago

fyi, @benjaminbhollon . I thought you might be interested in the way Hugo does templating.

benjaminbhollon commented 1 year ago

One thing to be aware of that I'm not sure whether or not you implemented in this is that navbar styles other than classy prefer a different HTML structure than was the previous recommendation—while classy needs a <span> wrapped around the <a> tags, the others work better without that.

More information is here: https://codeberg.org/Freedom-to-Write/readable.css/wiki/Navbar-Styles

benjaminbhollon commented 1 year ago

The <span> wrappers were just part of how the animated underline worked, and for other navbar styles they make the width of the links a bit odd (though it'll still work fine). :)

benjaminbhollon commented 1 year ago

Yeah, doesn't look like that was part of this.

benjaminbhollon commented 1 year ago

Okay so I'm not sure I did this right, but I think this would fix the issue:

Change

<nav data-style={{ default "classy" $.Site.Params.navbar_style }}>
  {{ range .Site.Menus.main -}}
  <span>
      <a href="{{ .URL | absLangURL }}">{{ .Name }}</a>
  </span>
  {{- end }}
</nav>

to

<nav data-style={{ default "classy" $.Site.Params.navbar_style }}>
  {{ range .Site.Menus.main -}}
  {{ if $.Site.params.navbar_style == "classy" }}
  <span>
  {{ end }}
      <a href="{{ .URL | absLangURL }}">{{ .Name }}</a>
  {{ if $.Site.params.navbar_style == "classy" }}
  </span>
  {{ end }}
  {{- end }}
</nav>

Keep in mind I don't really know Go templates, so I might have messed that up. Basic idea is, only include the <span> and </span> if the navbar style is classy.

rluetzner commented 1 year ago

Your code looks good. It's what I would have tried as well. 🙂 I read that the other styles no longer need span, but until now I didn't know that using span made them look weird. I have to admit that I did this without updating to 1.1.0, because for GitHub reasons it's not so simple to checkout your commits and I was a bit too lazy to do it myself. I verified the end result in the HTML, but couldn't try out different styles. I'll fix it this evening.

rluetzner commented 1 year ago

Done. I've cleaned up the code a bit, so there's less $.Site.params.navbar_style noise in there.

benjaminbhollon commented 1 year ago

I didn't know that using span made them look weird.

Yeah, it's due to the way that the links are spread out evenly, using flexbox. With classy, I want the links themselves to be narrow rather than super-wide, so I had it spread out the span element instead of the a element, so that the link underlines would only fit the text width. For the others, it either doesn't matter or I think it looks better without.

cjtheham commented 1 year ago

@rluetzner Looks like merging in #13 still caused a git conflict. Got a sec to fix this?

I'd also be happy if we included an example of the parameters, I think this would be an addition to the starter repo so I can add it, but what is the parameter formatting for YAML? Just navbar_style: 'classy'?

benjaminbhollon commented 1 year ago

Just navbar_style: 'classy'? From what I saw of the code, yeah that should be all it takes.

Feel free in whatever documentation/example you add to link here: https://codeberg.org/Freedom-to-Write/readable.css/wiki/Navbar-Styles

Right now it's mostly just implementation details, but I plan to add screenshots of the different styles.

rluetzner commented 1 year ago

Done. :slightly_smiling_face:

I was expecting this merge conflict, because Benjamin and I both edited the same line to add the navbar style. Because my changes weren't on top of Benjamin's commit, but rather before the update to v1.1.0, Git was unable to know which version of the line we want to keep.

I'm planning to add an example of this to the starter repo once this PR is merged. It's navbar_style: 'classy' (or any of the other values, which I'll link to), but you'll have to put it under parameters in your config file.

cjtheham commented 1 year ago

Looks good, merging this! Happy to also include whatever docs you'd like both here and on /hugo-starter-readable :D