feat(oui-navbar): add new components #235
Conversation
| const keysRegex = new RegExp([ | ||
| this.KEYBOARD_KEYS.TAB, | ||
| this.KEYBOARD_KEYS.SHIFT | ||
| ].join("|")); |
There was a problem hiding this comment.
Maybe this could be replaced by a templated string
There was a problem hiding this comment.
Yes, it could be
const keysRegex = new RegExp(`${this.KEYBOARD_KEYS.TAB}|${this.KEYBOARD_KEYS.SHIFT}`);
but i find this formulation more readable (and more elegant) :)
| const tabbableItems = this.navbarCtrl.getGroup(groupName); | ||
| const lastIndex = tabbableItems.length - 1; | ||
| const focusElement = (e, groupIndex) => { | ||
| let index = groupIndex; |
There was a problem hiding this comment.
I don't know if it's the wanted behaviour but groupIndex value will be changed too
There was a problem hiding this comment.
What do you mean ? With the recursive call ?
There was a problem hiding this comment.
@marie-j Number are passed by values and not by reference 😄 . Only Objects will be passed by reference.
| // Add a delay to force focus | ||
| const delay = 50; | ||
| this.$timeout(() => { | ||
| if (this.keyboardNav && this.keyboardNav[groupName] && this.keyboardNav[groupName][index]) { |
There was a problem hiding this comment.
Lodash could be used here 😉
There was a problem hiding this comment.
Old code when we did not wanted to integrate lodash in the project.
Good find ;)
| @@ -0,0 +1,119 @@ | |||
| <header class="oui-navbar-menu__header oui-navbar_mobile-only" | |||
| ng-if="::!!$ctrl.headerTitle"> | |||
| <h3 class="oui-navbar-menu__title" ng-bind="::$ctrl.headerTitle"></h3> | |||
There was a problem hiding this comment.
Shouldn't there be a h1 and h2 before ?
There was a problem hiding this comment.
It could even be a <p> or a <span>.
It's not important here :)
| </header> | ||
| <ul class="oui-navbar-menu__body oui-navbar-list"> | ||
| <li class="oui-navbar-menu__item oui-navbar-notification" | ||
| ng-repeat="menuLink in ::$ctrl.menuLinks | limitTo: $ctrl.limitTo" |
There was a problem hiding this comment.
I'll check to add that
| oui-navbar-group-last="::$last && !$ctrl.footerTemplate && !$ctrl.footerTitle"> | ||
| <div class="oui-navbar-notification__header"> | ||
| <span class="oui-navbar-notification__title" ng-bind=":: menuLink.subject"></span> | ||
| <span class="oui-navbar-notification__time" ng-bind="menuLink.time"></span> |
There was a problem hiding this comment.
Not here, the purpose is to update the time when needed (eg. 26m ago, 2h ago, etc.).
| </button> | ||
| </li> | ||
| <li class="oui-navbar-menu__item oui-navbar-notification oui-navbar-notification-placeholder" | ||
| ng-if="$ctrl.menuLinks.length === 0"> |
There was a problem hiding this comment.
Not here. It will be triggered if length is undefined, since $ctrl.menuLinks can be nullified.
| <div class="oui-navbar-notification-placeholder__title" | ||
| ng-bind="$ctrl.translations.notification.noNotification"></div> | ||
| <div class="oui-navbar-notification-placeholder__description" | ||
| ng-bind="$ctrl.translations.notification.noNotificationDescription"></div> |
There was a problem hiding this comment.
I think JD forgot those ones. Since it's only for placeholder, it could be one-time binded :)
|
@marie-j updated :) |
| ng-class="::'navbar-' + $ctrl.iconClass"> | ||
| <span class="oui-icon__badge" | ||
| ng-if="!!$ctrl.iconBadge" | ||
| ng-bind="$ctrl.iconBadge"> |
There was a problem hiding this comment.
Not here. What if the value of the badge is updated ? :)
There was a problem hiding this comment.
This is why I was asking a question 😄
| const tabbableItems = this.navbarCtrl.getGroup(groupName); | ||
| const lastIndex = tabbableItems.length - 1; | ||
| const focusElement = (e, groupIndex) => { | ||
| let index = groupIndex; |
There was a problem hiding this comment.
@marie-j Number are passed by values and not by reference 😄 . Only Objects will be passed by reference.
| export default { | ||
| ALT: 9, | ||
| TAB: 16, | ||
| TAB: 9, |
There was a problem hiding this comment.
Could be used elsewhere, don't we have a package for this kind of universal code ?
There was a problem hiding this comment.
Do we have to import a package just for 3 values ?
|
|
||
| // Return value of "ui-sref" | ||
| getFullSref () { | ||
| return `${this.state}(${JSON.stringify(this.stateParams)})`; |
There was a problem hiding this comment.
I don't get this piece of code, isn't this.stateParams supposed to be a String ? If it is, maybe you could throw an error if it isn't a String
There was a problem hiding this comment.
No, stateParams is an object
|
|
||
| addItemToGroup (item, groupName) { | ||
| // Create the group if it doesn't exist | ||
| if (angular.isUndefined(this.keyboardNav[groupName])) { |
There was a problem hiding this comment.
this.keyboardNav[groupName] === undefined ?
There was a problem hiding this comment.
Yes, it's the same :)
| // Set focus to an item of a group | ||
| setFocusTo (groupName, index) { | ||
| // Add a delay to force focus | ||
| const delay = 50; |
There was a problem hiding this comment.
I really don't have another solution but this is 😒 , wouldn't autofocus work ?
There was a problem hiding this comment.
Not the same, we don't want to autofocus on the page load
| this.togglerLoaded = !!changes.togglerLinks.currentValue; | ||
| } | ||
| $onDestroy () { | ||
| this.$document |
There was a problem hiding this comment.
We've got listeners on the whole document ? What are they for ?
There was a problem hiding this comment.
keydown for ESC and click to close opened menu :)
AxelPeter
force-pushed
the
feature/upgrade-navbar
branch
from
5dae062 to
d37011a
Compare
|
I have done a rebase for the conflicts |
There was a problem hiding this comment.
Nice job @AxelPeter!
Do not forget to squash your commits:)
|
@antleblanc Thanks, I know ;) |
4 participants
http://build_upgrade_navbar.ui-kit.ovh/#!/oui-angular/navbar
Linked to ovh/ovh-ui-kit#246