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.

Sign up for GitHub

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

Jump to bottom

feat: Support Hero Icons in the nav bar menu #359

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −0
Open

feat: Support Hero Icons in the nav bar menu #359

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions layouts/partials/navbar.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@
{{ if $external }}target="_blank" rel="noreferer"{{ end }}
class="hx-text-sm contrast-more:hx-text-gray-700 contrast-more:dark:hx-text-gray-100 hx-relative -hx-ml-2 hx-hidden hx-whitespace-nowrap hx-p-2 md:hx-inline-block {{ $activeClass }}"
>
{{ with .Params.hero_icon -}}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{ with .Params.hero_icon -}}
{{ with .Params.icon -}}

I'd prefer icon over hero_icon

Copy link
Contributor Author

@dogweather dogweather Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, but .icon sets it as icon only, I believe. That's why I went with hero_icon.

A nice enhancement for clarity might be to change icon to icon_item and deprecate icon.

Copy link
Contributor Author

@dogweather dogweather Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, we could refactor this even more. Personally, I do like "making illegal states impossible". Although this gets a little bulky.

The docs describe this as four page types. Three of them are implicitly defined. We could use type for these as well (it's already used for type: search). That could look like this:

- name: github
  url:  "https://github.com/dogweather"
  params:
    type: icon    // One of: pageref, url, search, icon
    icon: github  // Required for "icon" type. Defaults to question-mark-circle

- name: forum
  url:  "https://forum"
  params:
    type: url
    icon: chat-alt-2

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of using an extra type there, let's consider these cases

# Icon only, for backward compatibility
- name: github
  url: https://github.com/
  params:
    icon: github

# Link with icon, must set `type: link` and `icon`
- name: forum
  params:
    type: link
    icon: chat-alt-2

# Search bar only
- name: Search
  params:
    type: search
    icon: chat-alt-2 # undefined behavior, currently ignored

In the future, we may extend the type to other widgets, for example, light/dark/system theme toggle switch:

- name: Theme toggle
  params:
    type: theme-toggle

<span style="float: left; padding-right: 0.3em; padding-top: 2px; opacity: 0.75">
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use Tailwind CSS classes here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet we could. I was hoping you'd know what they are. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can begin converting them.

{{ partial "utils/icon.html" (dict "name" . "attributes" "height=18") }}
</span>
{{- end }}

<span class="hx-text-center">{{ or (T .Identifier) .Name | safeHTML }}</span>
</a>
{{- end -}}
Expand Down