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

Add support for nested menus #781

Merged
merged 9 commits into from
Jul 10, 2020

Conversation

mpourismaiel
Copy link
Member

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

@mpourismaiel mpourismaiel added this to the v0.18.0 milestone Jun 12, 2020
@mpourismaiel mpourismaiel requested a review from stp-ip June 12, 2020 18:19
@mpourismaiel mpourismaiel self-assigned this Jun 12, 2020
@stp-ip
Copy link
Member

stp-ip commented Jun 25, 2020

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

@stp-ip
Copy link
Member

stp-ip commented Jun 29, 2020

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
Copy link
Member Author

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
Copy link
Member

stp-ip commented Jul 1, 2020

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
Copy link
Member Author

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
Copy link
Member

stp-ip commented Jul 2, 2020

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
Copy link
Member Author

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

@stp-ip
Copy link
Member

stp-ip commented Jul 2, 2020

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
Copy link
Member Author

mpourismaiel commented Jul 2, 2020

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
Copy link
Member

stp-ip commented Jul 2, 2020

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

@mpourismaiel
Copy link
Member Author

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
Copy link
Member

stp-ip commented Jul 2, 2020

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
Copy link
Member Author

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
Copy link
Member

stp-ip commented Jul 3, 2020

Sounds good.

@mpourismaiel
Copy link
Member Author

Is there anything else we can do on this PR?

@stp-ip
Copy link
Member

stp-ip commented Jul 3, 2020

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

stp-ip
stp-ip previously approved these changes Jul 3, 2020
Copy link
Member

@stp-ip stp-ip left a comment

Choose a reason for hiding this comment

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

edit: damn wrong tab

@mpourismaiel
Copy link
Member Author

I thought I pushed the commits. Sorry :D

@mpourismaiel
Copy link
Member Author

Also created tests within #791

Comment on lines 89 to 118
[[menu.main]]
url = "/dev"
name = "Dev Section"
weight = 35
identifier = "dev"

[[menu.main]]
url = "/dev"
name = "Dev Section"
weight = 10
parent = "dev"

[[menu.main]]
url = "/dev/colors"
name = "Colors"
weight = 20
parent = "dev"

[[menu.main]]
url = "/dev/blog"
name = "Blog"
weight = 30
parent = "dev"

[[menu.main]]
url = "/dev/events"
name = "Events"
weight = 40
parent = "dev"

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I agree with having them in the normal config.toml as well.
That one is more likely to be copied such as to our demo page, which doesn't use the /dev/ pages.

Any reason why you added them here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

We aren't using config.toml in Netlify and it's not used in package.json as well. I thought since it's not in any of those files it's not used at all so updated it just to keep it in line with config-dev.toml. I removed the menus from it now.

@stp-ip
Copy link
Member

stp-ip commented Jul 9, 2020

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.

@stp-ip stp-ip merged commit 6f92247 into okkur:master Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

[nav] Support dropdown menu if hasChildren
2 participants