Add pfVerticalNavigation directive. - #276
Conversation
|
@dgutride @dtaylor113 Please review Please try out the NG Docs for this and give me any usability thoughts. Due to the full page nature of vertical navigation, I had to take some liberties to be able to demo this in the NG Docs. |
| * Directive for vertical navigation. This sets up the nav bar header with the collapse button (hamburger) and the | ||
| * application brand image (or text) as well as the vertical navigation bar containing the navigation items. This | ||
| * directive supports primary, secondary, and tertiary navigation with options to allow pinning of the secondary and | ||
| * tertiary navigation menus as well as teh option for persistent secondary menus. |
|
Pulled/locally - built and ran without issues. One thought on the demo area - I like how it takes you in and out of that mode, but maybe a small bit of text explaining that they will be taken to a new window and can return from there could make it a little less jarring. The top button once inside was very clear, though so it's pretty obvious how they can get back there. One other option is to launch it in a new window (if that's possible) as long as we tell them we are doing that. Just one spelling issue I saw in the help text. |
|
@dgutride I thought about launching a window but I'm unsure how to get that to work such that the files are served up for the new window. All the components are added via the ng-docs and I don't know how to make it available to a new window. I'm not exactly sure what I could add for text to explain that the current content of the page will be hidden while in demo mode. |
1494dbf to
bab1557
Compare
|
Updated to fix @dgutride issue (mis-spelling) |
| }; | ||
|
|
||
| if (!$scope.updateActiveItemsOnClick) { | ||
| rootScope.$on( "$routeChangeSuccess", function (event, next, current) { |
There was a problem hiding this comment.
This event should be disposed of when the directive is removed.
|
Looks good to me. +1 to perhaps adding a little more text indicating whats going to happen when a user clicks on the [Show Visual Navigation] button |
bab1557 to
f30af3c
Compare
|
Added call to remove route change listener on destroy ( @dgutride issue above ) |
f30af3c to
8117714
Compare
Directive for vertical navigation. This sets up the nav bar header with the collapse button (hamburger) and the application brand image (or text) as well as the vertical navigation menu containing the navigation items. This directive supports primary, secondary, and tertiary navigation with options to allow pinning of the secondary and tertiary navigation menus as well as the option for persistent secondary menus.
|
Added a blurb to forewarn about the content being covered and how to get back ( @dgutride @dtaylor113 comments ). |
|
This looks good to me. |
|
@dtaylor113 Please re-review |
|
LGTM |
Add pfVerticalNavigation directive.
Directive for vertical navigation.
This sets up the nav bar header with the collapse button (hamburger)
and the application brand image (or text) as well as the vertical
navigation menu containing the navigation items.
This directive supports primary, secondary, and tertiary
navigation with options to allow pinning of the secondary and
tertiary navigation menus as well as the option for persistent
secondary menus.