[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: |
|
||||||||||||
| 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:
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. Reproduce:
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(); |