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.

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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dogweather
Copy link
Contributor

A Hugo menus config with params.hero_icon:

menus:
  main:
  - identifier: discussions
    name: Forum
    url: https://github.com/dogweather/forkful/discussions
    weight: 1
    params:
      hero_icon: chat-alt-2

...displays the icon next to the text.

Screenshot 2024-04-15 at 16 24 41

I used GitHub and OpenAI as design examples for size and distance to the text. Here are more examples of combinations of icons/non-icons:

Screenshot 2024-04-15 at 16 24 16 Screenshot 2024-04-15 at 16 23 52

This code is running live here: https://forkful.ai/en/

Copy link

netlify bot commented Apr 15, 2024

Deploy Preview for hugo-hextra ready!

Name Link
🔨 Latest commit bb5923f
🔍 Latest deploy log https://app.netlify.com/sites/hugo-hextra/deploys/661dab6daf4fb60008f5dc23
😎 Deploy Preview https://deploy-preview-359--hugo-hextra.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Owner

@imfing imfing left a comment

Choose a reason for hiding this comment

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

Thank you, this is a nice feature.
Let's enhance the implementation slightly.

@@ -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

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

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

@@ -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 -}}
<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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants