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

Siblings of active and node ID fixes. #35

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Siblings of active and node ID fixes. #35

wants to merge 3 commits into from

Conversation

imagehat
Copy link

First of all, thanks for the great plugin!

In dealing with a few more complex site navigations I ran into a couple issues, it would be great if you could review my fixes and see if you would roll them back in to Navee.

Issue 1

startWithSiblingsOfActive was showing nodes at the same level but outside of the current branch.

Full menu:

- Item 1
    - Item 1.1 
        - Item 1.1.1
        - Item 1.1.2
            - Item 1.1.2.1 (ACTIVE NODE)
            - Item 1.1.2.2
        - Item 1.1.3
            - Item 1.1.3.1
            - Item 1.1.3.2
    - Item 1.2
- Item 2
    - Item 2.1
    - Item 2.2

Unexpected problem:

- Item 1.1.2.1 (ACTIVE NODE)
- Item 1.1.2.2
- Item 1.1.3.1
- Item 1.1.3.2

Expected behavior and fix: only show direct siblings in the same branch.

- Item 1.1.2.1 (ACTIVE NODE)
- Item 1.1.2.2

Issue 2

startWithNodeId and startWithChildrenOfNodeId was not working unless there was an active node.

Full menu (no active nodes):

- Item 1 (node id 98)
    - Item 1.1 
    - Item 1.2
- Item 2  (node id 99)
    - Item 2.1
    - Item 2.2

Unexpected problem (with parameter of startWithChildrenOfNodeId: 98 ):

- Item 1.1
- Item 1.2
- Item 2.1
- Item 2.2

Expected behavior and fix: nav should always start with the node id specified, even if there is no currently active node at all.

- Item 1.1
- Item 1.2

Moving the startWithNodeId and startWithChildrenOfNodeId outside of the active nodes conditional because these parameters don't actually require an active node.
@ian-dcuk
Copy link

@imagehat I ran into Issue 2 as well - thanks for the fix!

Additionally, in this block I believe the startWithNodeId branch should use < and > while the startWithChildrenOfNodeId branch should use <= and >= when unsetting nodes so that the root node is retained or removed as expected.

@mgcross
Copy link

mgcross commented Oct 18, 2022

Thanks @imagehat for the fix, I was banging my head on a legacy craft site! And thank you also @ian-dcuk for the additional fix - that resolved the issue of the parent nav element being output as well as children.

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.

3 participants