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: customMenus in footer #513

Merged
merged 3 commits into from
Jan 31, 2022

Conversation

stueja
Copy link
Contributor

@stueja stueja commented Jan 27, 2022

Show customMenu links also in footer.
In config.yaml,

  • params.footer.enable is true to show the footer at all
  • params.footer.navigation.enable is true to show navigation items
    in the footer
  • params.footer.navigation.customMenus.enable is true to show
    customMenu items

Example:

params:
  footer:
    enable: true
    navigation:
      enable: true
      customMenus:
        enable: true

In data/<language>/site.yaml,

  • customMenus must contain at least one entry with showOnFooter: true

Example:

customMenus:
  - name: Imprint
    url: posts/imprint
    showOnFooter: true

Issue

Description

Test Evidence

Show customMenu links also in footer.
In `config.yaml`,
 - `params.footer.enable` is `true` to show the footer at all
 - `params.footer.navigation.enable` is `true` to show navigation items
   in the footer
 - `params.footer.navigation.customMenus.enable` is `true` to show
   customMenu items

Example:
```
params:
  footer:
    enable: true
    navigation:
      enable: true
      customMenus:
        enable: true
```

In `data/<language>/site.yaml`,
 - `customMenus` must contain at least one entry with `showOnFooter:
   true`

Example:
 ```
 customMenus:
   - name: Imprint
     url: posts/imprint
     showOnFooter: true
 ```
@hossainemruz
Copy link
Member

@stueja I think we should avoid another extra layer of nesting in the footer. We can just specify,

params:
  footer:
    enable: true
    navigation:
      enable: true
      customMenus: true

Also, do we really need showOnFooter field in the custom menu entry? Why user might want to keep some menu items hidden in footer where he allowed in navbar?

@stueja
Copy link
Contributor Author

stueja commented Jan 27, 2022

Some items might not be wished in the top navbar, e. g. imprint or GDPR declaration or other. This was my thought.
I had implemented that in my local copy and thought I'd share it with you. But of course it is up to you to decide which PR you want to merge and which one not :)

@hossainemruz
Copy link
Member

hossainemruz commented Jan 28, 2022

Ok. That's a valid use case. I will review the PR soon. Can you please acknowledge the nesting issue on footer.navigation.customMenus ?

But of course it is up to you to decide which PR you want to merge and which one not :)

Sorry, if I disappointed you. We have to keep balance between features and maintainability. If there are too many moving parts, chances are something will break in every release. That's why we have to decide whether a use-case is worth or not to take the burden.

@stueja
Copy link
Contributor Author

stueja commented Jan 28, 2022

Absolutely no disappointment on my side! I just wanted to say that I don't have hard feelings if a PR is not merged.
I will change the nesting also once I found out how to change a PR.
Edit: done

Copy link
Contributor

@Rihoj Rihoj left a comment

Choose a reason for hiding this comment

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

I personally like this change, and it looks like the nested issue mentioned was taken care of. Is there any way we could tie this to a change to https://github.com/hugo-toha/guides and https://toha-guides.netlify.app/posts/configuration/site-parameters/#add-custom-menus to try to keep the documentation up to date?

@hossainemruz
Copy link
Member

@Rihoj Thank you for reviewing. I am looking for a spare time to update those guides before merging this PR. It would be greatly appreciated if you can send a PR for the guides. I can merge this PR then.

stueja pushed a commit to stueja/guides that referenced this pull request Jan 30, 2022
@kodiakhq kodiakhq bot merged commit a159ff6 into hugo-toha:main Jan 31, 2022
kodiakhq bot pushed a commit to hugo-toha/guides that referenced this pull request Jan 31, 2022
Update documentation for
hugo-toha/toha#513
and
hugo-toha/toha#514

Co-authored-by: stueja <[email protected]>
Co-authored-by: Emruz Hossain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants