-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Patternfly vertical navigation and project bar #1932
Conversation
@@ -24,38 +24,38 @@ angular.module('openshiftConsole') | |||
dropdownItems = dropdownItems.concat([{ | |||
type: 'dom', | |||
node: [ | |||
'<li>', | |||
'<li class="list-group-item">', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break existing extensions if we require a new class on menu items.
@rhamilto @jeff-phillips-18 Any ideas on how we can avoid that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Options:
-
Duplicate the PatternFly's LESS rules related to styling the mobile nav in the console's LESS. Poses a maintenance challenge and duplicates rules.
-
Use JS to add the classes.
Both options are not ideal, but I think I favor the second one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've also got a bug that's related. As it stands, primary nav links need an ng-click="nav.showMobileNav = false;" to correctly close the nav at mobile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have this fixed except for active styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The active styles work now too
6338ea7
to
13bd176
Compare
@dtaylor113 FYI, we should be able to update the application launcher menu to match the mobile design when this lands. I suggest we do it as a follow on to this PR. |
d579700
to
9d2ae6f
Compare
app/styles/_variables.less
Outdated
@@ -7,7 +7,15 @@ | |||
// Patternfly overrides | |||
@dropdown-divider-margin: 4px 0; | |||
@list-view-hover-bg: #fafafa; | |||
@navbar-pf-alt-navbar-brand-padding: 20px 0; | |||
@navbar-pf-height: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually not used, needs to be removed
app/styles/_variables.less
Outdated
@navbar-os-header-height-mobile: 46px; | ||
@navbar-header-right-margin: 10px; | ||
@navbar-os-header-desktop: 59px; | ||
@navbar-os-header-mobile: 39px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove height
from the var name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't recall why 🤔 but shouldn't have been.
512ac26
to
42c5218
Compare
73d5ae9
to
4435b27
Compare
I've added some basic accessibility support to the nav. Tested using the keyboard and iOS simulator. |
69fa11d
to
6176ddb
Compare
app/views/_sidebar.html
Outdated
</a> | ||
|
||
<!-- mobile secondary nav --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is just for mobile? It's secondary nav for desktop, too, right?
If so, let's update the comment.
@jeff-phillips-18 Let me know if you'd like to review this. It's a large PR, but the views are mostly whitespace changes and removed elements. The important changes are in nav.js, header.html, and _sidebar.html -- and then of course many changes to the less styles. @rhamilto Is working on a few last items to show active nav for pages that didn't used to have a left nav. We are showing this at the design review today to verify what we've done with @openshift/team-ux-review We opted to go with base Patternfly to keep compatibility with our extension points. We have about 5 extension points in the masthead and left nav that we must keep supporting without breaking existing extensions. I'd like to give this PR review priority if possible. For one, if we merge it earlier in the sprint, it has more soak time. It's also difficult to rebase since it touches many files. @rhamilto Can you review the less changes from @sg00dwin? @openshift/team-ux-review I'm really happy with the UX of the new nav. Thanks! This PR also removes more code than it adds, while adding function, which is great. |
@abhgupta Heads up that nav changes are likely coming this sprint. You should not need to change any of the online extensions, but we should verify. |
app/styles/_vertical-nav.less
Outdated
margin-left: 20px; | ||
} | ||
.nav-vertical-primary { | ||
display: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should use an ng-if
for this rather than hiding the vertical nav in CSS
https://github.com/openshift/online/pull/1408 is required for the new nav to work correctly with online. |
2111a81
to
c2913bf
Compare
Cleaned up indentation in the views, squashed, and added dist |
c2913bf
to
2b2df05
Compare
[test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great teamwork on this one!
Looks like we have some test failures to work through :) |
@sg00dwin We should try to avoid the double scrollbars. I'm not sure the sidebar needs to scroll separately. (Can be a follow on.) |
Update navigation to use the standard Patternfly vertical navigation. Add a project bar to the masthead when in the context of a project.
aea7d40
to
bc13e0e
Compare
Tests are passing. Thanks everyone. [merge] |
Evaluated for origin web console test up to bc13e0e |
Origin Web Console Test Results: Running (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/111/) (Base Commit: 4be3793) (PR Branch Commit: bc13e0e) |
Evaluated for origin web console merge up to bc13e0e |
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/95/) (Base Commit: 4be3793) (PR Branch Commit: bc13e0e) |
https://trello.com/c/ZcLBaLK7
This is #1929, rebased
@sg00dwin I've left you as the commit author.
@rhamilto FYI
TODOs:
localStorage
Follow on: