Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

View\Helper\Navigation\Menu: add flag to set page class to <li> #4298

Closed
wants to merge 2 commits into from

Conversation

Slamdunk
Copy link
Contributor

In ZF1 there was the possibility to put the page class to <li> instead of internal <a>:

https://github.com/zendframework/zf1/blob/release-1.12.3/library/Zend/View/Helper/Navigation/Menu.php#L103-L108

Before:

<ul class="navigation">
    <li>
        <a class="foobar" href="site2">Site 2</a>
        <ul>
            <!-- [...] -->
        </ul>
    </li>
</ul>

After:

<ul class="navigation">
    <li class="foobar">
        <a href="site2">Site 2</a>
        <ul>
            <!-- [...] -->
        </ul>
    </li>
</ul>

This behaviour is necessary to correctly style the internal <ul> of a specific <a> page.

Tests pass, I'll rebase when global test fail will be fixed.

@vnagara
Copy link
Contributor

vnagara commented Apr 23, 2013

I guess you should work around naming like: aClass (anchor), liClass(list). And functions: addAClass, addLiCalss, set[...]. Also you could put in them some default class(may take them from boostrap css).
I guess there should be done some routine work about management of css classes.

@froschdesign
Copy link
Member

@vnagara

I guess you should work around naming like: aClass (anchor), liClass(list). And functions: addAClass, addLiCalss…

  • Use the "descendant selector" in CSS and you do not need a class on both elements
  • Very ugly names.

Also you could put in them some default class(may take them from boostrap css).

No default values! If you use the "Foundation" framework, the class names from "Bootstrap" are wrong.

*
* @var bool
*/
protected $addPageClassToLi = false;
Copy link
Member

Choose a reason for hiding this comment

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

Why the verbiage "page"? Why not simply $addClassToLi?

Copy link
Member

Choose a reason for hiding this comment

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

Also, expand "Li" to "ListItem"; abbreviations without context are often difficult to understand when just glancing through code.

Copy link
Member

Choose a reason for hiding this comment

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

@weierophinney
It is the old from version 1.12.

Copy link
Member

Choose a reason for hiding this comment

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

@weierophinney
"ListItem" 👍

@weierophinney
Copy link
Member

Looks sane -- make the changes requested, and I'll merge for 2.2.0.

@froschdesign
Copy link
Member

👍

@ghost ghost assigned weierophinney Apr 25, 2013
weierophinney added a commit that referenced this pull request Apr 25, 2013
View\Helper\Navigation\Menu: add flag to set page class to <li>

Conflicts:
	library/Zend/View/Helper/Navigation/Menu.php
weierophinney added a commit that referenced this pull request Apr 25, 2013
- Per php-cs-fixer
weierophinney added a commit that referenced this pull request Apr 25, 2013
@weierophinney
Copy link
Member

Merged to develop for release with 2.2.0.

@Slamdunk Slamdunk deleted the nav/addPageClassToLi branch April 26, 2013 05:44
weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015
…/addPageClassToLi

View\Helper\Navigation\Menu: add flag to set page class to <li>

Conflicts:
	library/Zend/View/Helper/Navigation/Menu.php
weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants