[MGNLSTK-1300] Vertical Navigation isShowVerticalNavigation returns incorrect value Created: 15/Nov/13  Updated: 03/Feb/14  Resolved: 03/Feb/14

Status: Closed
Project: Magnolia Standard Templating Kit (closed)
Component/s: None
Affects Version/s: 2.7
Fix Version/s: 2.0.17, 2.7.2

Type: Bug Priority: Major
Reporter: Ricardo Ulate Assignee: Philip Mundt
Resolution: Fixed Votes: 0
Labels: maintenance, next, quickwin, support
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLSTK-1178 Start level of subnavigation not corr... Closed
relation
Template:
Patch included:
Yes
Acceptance criteria:
Empty
Date of First Response:

 Description   

The method createVerticalNavigation() correctly checks the start level, but the method isShowVerticalNavigation() does not. Affecting everything that uses the vertical navigation.

    public boolean isShowVerticalNavigation() {
        try {
            if(isShowHorizontalNavigation() && siteRoot.getDepth() + this.getHorizontalLevel() > currentNode.getDepth() ){
                return false;
            }
            final boolean enabled = siteNavigation.getVertical().getEnabled();
            final boolean empty = createVerticalNavigation().getItems().isEmpty();
            return enabled && !empty;
        }
        catch (RepositoryException e) {
            log.error("Can't check existence of vertical navigation",e);
            return false;
        }
    }

Should be something like:

    public boolean isShowVerticalNavigation() {
        int startlevel = siteNavigation.getVertical().getStartLevel() > 0 ? siteNavigation.getVertical().getStartLevel() : startLevel + getHorizontalLevel();
        try {
            if(isShowHorizontalNavigation() && siteRoot.getDepth() + startLevel > currentNode.getDepth() ){
                return false;
            }
            final boolean enabled = siteNavigation.getVertical().getEnabled();
            final boolean empty = createVerticalNavigation().getItems().isEmpty();
            return enabled && !empty;
        }
        catch (RepositoryException e) {
            log.error("Can't check existence of vertical navigation",e);
            return false;
        }
    }


 Comments   
Comment by Christopher Zimmermann [ 29/Jan/14 ]

Fix isShowVerticalNavigation to take startLevel setting of verticalNavigation into account so that for example the horizontal and vertical navigation can be displayed at the same time.

Example of what did not work properly previously, but now works:
On a fresh install of STK and demo project:

  • Open Site Definitions App
  • Navigate to: /templates/prototype/navigation/vertical and add property startLevel=1
  • Navigate to: /templates/prototype/navigation/horizontal and set property level=2
  • In Pages app open /demo-project/about
    > With the fix the horizontal nav will be displayed on top and the vertical nav will be displayed to the left. Without the fix, the vertical nav will not be displayed.

Note, also see for reference: http://documentation.magnolia-cms.com/display/DOCS/Navigation

Comment by Milan Divilek [ 31/Jan/14 ]

Reopen:

1. If you compare determination of start level in info.magnolia.module.templatingkit.navigation.SiteNavigationModel#isShowVerticalNavigation and in info.magnolia.module.templatingkit.navigation.SiteNavigationModel#createVerticalNavigation then it's different.
In #createVerticalNavigation() method if siteNavigation.getVertical().getStartLevel() > 0 then siteNavigation.getVertical().getStartLevel() is used as startLevel, but in #isShowVerticalNavigation method siteRoot.getDepth() + siteNavigation.getVertical().getStartLevel() is used as startLevel. It causing wrong vertical navigation when siteRoot (Home page) is not on top level. Correctly we should use siteRoot.getDepth() + siteNavigation.getVertical().getStartLevel() in both of them.

Reproduce:

  • in Site Definitions App add property startLevel=1 to /templates/prototype/navigation/vertical
  • Create new top level site called firstLevel with template "redirect"
  • Create sub page called secondLevel with template "redirect"
  • Move whole demo-project under secondLevel
  • Open /firstLevel/secondLevel/demo-project/about site and you can see wrong vertical navigation. It contains menu secondLevel and demo-project items

2. Because the determination of startLevel should be same in both (#isShowVerticalNavigation, #createVerticalNavigation) method we should consider to create new private method for determination of startLevel and then use this method in both methods.

3. Why did you create new isShowVerticalNavigationPrevious() method and let there old code? If the old code is wrong then we should remove it.

4. I would do also the backport into 2.0.x version (Magnolia 4.5.x). See discussion under http://documentation.magnolia-cms.com/display/DOCS/Navigation . It should be easy port, because there should not be difference between code.

Comment by Milan Divilek [ 03/Feb/14 ]

Reopen:

If no vertical start level was set then you do startLevel += getHorizontalStartLevel() + getHorizontalLevel(); , but problem is getHorizontalStartLevel() this property is not used during creation of info.magnolia.module.templatingkit.navigation.SiteNavigationModel#getHorizontalNavigation, because horizontal menu has to begin on siteRoot.

Correct code should be only startLevel += getHorizontalLevel();

Generated at Mon Feb 12 07:35:16 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.