Add isActive method Navigation Page Uri. #4348

Closed
wants to merge 14 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

tomshaw commented Apr 28, 2013

This PR adds active links to Zend Navigation pages generated using the uri container key. This is an attempt to fix #4346

Member

DASPRiD commented Apr 28, 2013

  1. Why injecting the router, when injecting the request object is enough?
  2. The URI navigation page can not just contain a path, but query parameters, hostname, scheme and such as well. Those should be taken into account.
Contributor

tomshaw commented Apr 28, 2013

I like the idea of using the request object, the router was already available thats why I used it. How would you match the active route with only the uri string to go by, that was the dilemma i had trying to make the active route active css wise. The router its so much more definitive than a uri string but the above works for simple use cases.

Member

froschdesign commented Apr 29, 2013

@tomshaw

  1. I think, we should only have a "soft" dependency to other ZF components for the page type "URI". So that the Zend\Navigation component can also be used outside of a ZF-MVC application.
  2. A "soft" dependency to the request object is enough.
  3. Please do not forgot unit tests.
  4. Thanks! 😄
Owner

weierophinney commented Apr 29, 2013

I agree with @DASPRiD and @froschdesign here. If you can make the changes in the by Tuesday at noon GMT, I'll consider them for 2.2.0; otherwise, we'll have to delay them to 2.3.0.

Contributor

tomshaw commented Apr 29, 2013

Awesome! I'm not an expert with phpunit but I'll try my best. :)

Member

froschdesign commented Apr 30, 2013

Please add one clean commit! There are to many forward and backward steps in your commits.

Please do not change this!

The length of the line was good. Why you change this?

Please do not change this!

This is a hard dependency to the request class. Please create a soft dependency:

if ($this->getRequest() instanceof RequestInterface) {
    // then check the URI
}
Owner

tomshaw replied Apr 30, 2013

@froschdesign I've created the soft dependency but I'm slightly confused. Your recommending using the RequestInterface? By the way thanks for the input!

Please do not change this!

Owner

tomshaw commented on 9bd6f50 Apr 30, 2013

@froschdesign My bad I used Zend Studio 10 PSR-2 code format .

@weierophinney weierophinney added a commit that referenced this pull request May 22, 2013

@weierophinney weierophinney Merge pull request #4348 from tomshaw/zf4346
Add isActive method Navigation Page Uri.
696b4ff

@weierophinney weierophinney added a commit that referenced this pull request May 22, 2013

@weierophinney weierophinney [#4348] Fixed test errors
- Only inject request if it is an HTTP request
- Allow injecting a null value for request in URI page type
d4b20c0

@weierophinney weierophinney added a commit that referenced this pull request May 22, 2013

@weierophinney weierophinney [#4348] CS fixes
- indentation
- trailing whitespace
8e072bb

@weierophinney weierophinney added a commit that referenced this pull request May 22, 2013

@weierophinney weierophinney Merge branch 'feature/4348' into develop
Close #4348
1bf70a6
Owner

weierophinney commented May 23, 2013

Merged to develop for release in 2.3.0.

@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge pull request zendframework/zendframework#4348 from tomshaw/zf4346
Add isActive method Navigation Page Uri.
86e412d

@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015

@weierophinney weierophinney [zendframework/zendframework#4348] Fixed test errors
- Only inject request if it is an HTTP request
- Allow injecting a null value for request in URI page type
bd0af13

@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015

@weierophinney weierophinney [zendframework/zendframework#4348] CS fixes
- indentation
- trailing whitespace
fc4d8b7

@weierophinney weierophinney added a commit to zendframework/zend-navigation that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'feature/4348' into develop 5559cd9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment