okkur / syna

Highly customizable open source theme for Hugo based static websites
https://syna.okkur.org/demo/
Apache License 2.0
250 stars 134 forks source link

Add support for nested menus #781

Closed mpourismaiel closed 4 years ago

mpourismaiel commented 4 years ago

What this PR does / why we need it: Add support for nested main menus in nav fragment

Which issue this PR fixes: fixes #683

Release note:

- nav: Add support for nested menus by adding dropdowns
stp-ip commented 4 years ago

We should at least have an example within our docs and maybe even a test case.

stp-ip commented 4 years ago

What do you think about adding a link back to the nesting docs from Hugo at least as a reference or footnote. https://gohugo.io/content-management/menus/#nesting

What's your stance on a test for this?

mpourismaiel commented 4 years ago

In order to add a test we have to create a new website or add nesting menus to our demo. Is there a menu we want in the demo that can be nested?

stp-ip commented 4 years ago

One idea would be an additional nav for dev with a few nested pages for colors for example. That would test multiple navy as well as nesting. Thoughts?

mpourismaiel commented 4 years ago

Problem is menus are global, adding a nested menu would add it everywhere. Is it okay to add a menu to every page to link to colors page?

stp-ip commented 4 years ago

Can't we do something in config-dev.toml such as:

  [[menu.dev]]
    url = "/dev"
    name = "Dev"
    weight = 10
    identifier = "dev"

  [[menu.dev]]
    url = "/dev/colors/<fragment>"
    name = "Color <fragment>"
    weight = 10

  [[menu.dev]]
    url = "/dev/colors/<fragment>"
    name = "Color <fragment>"
    weight = 20

Not sure why this needs to be global?

mpourismaiel commented 4 years ago

Only menu.main supports dropdown in nav. Do we allow for custom menus in nav fragment?

stp-ip commented 4 years ago

Ahh good point. Missed that nested is done via menu.main.

Within the config-dev.toml we could add the dev section within the main menu I assume, which then has a nested menu. What do you think? Would be helpful I guess.

Currently we don't allow for custom menus in nav yet, but with multiple navs on the same page, we probably should or?

mpourismaiel commented 4 years ago

As you mentioned in the past, menus offer unique features from Hugo that we can't replicate. As we don't use most of them for now, we can allow a custom menu and disabling the main menu in nav fragment which would be helpful in some situations. But nav fragment already has too many options and adding a new one might make the complexity too much. Not sure though.

I will add a dev menu to the navbar which would allow us to create tests for the dropdown. We can discuss the addition of custom menus in another issue if you agree.

stp-ip commented 4 years ago

Damn I love that you just let me reflect and reconsider adding another option. Good points. Let's discuss this in another issue.

mpourismaiel commented 4 years ago

Haha. Also I think testing shouldn't be done here. We should however write tests for our bootstrap and jquery helpers. Should I create a tracking issue?

stp-ip commented 4 years ago

What's your reasoning behind not adding tests? As this is a Hugo function and bootstrap, which should work or be tested on their side?

mpourismaiel commented 4 years ago

The nested menu is a Hugo feature and works from their side, our testing would be on top of theirs which seems redundant. But dropdown, although taken from bootstrap and using bootstrap styles, is implemented by us and therefore should be tested. And since there are other functionalities like it implemented, I think writing tests for all of them in a separate PR is cleaner.

stp-ip commented 4 years ago

Sounds good.

mpourismaiel commented 4 years ago

Is there anything else we can do on this PR?

stp-ip commented 4 years ago

Thought we still wanted to add the nested menu into config-dev.toml or did we want this to happen in a separate PR?

mpourismaiel commented 4 years ago

I thought I pushed the commits. Sorry :D

mpourismaiel commented 4 years ago

Also created tests within #791

stp-ip commented 4 years ago

Tests can be approved separately then as you mentioned they are in the jq PR.

Let's clear up the above comment on config.toml inclusion and then get this merged.