Put the options of the title bar in a menu #95
Conversation
|
I think the borders around the icon menu are making the menu bar too busy. |
There was a problem hiding this comment.
We'll keep the borders for now. There is a bug on them: they're too tall. I think that once the bug is fixed, borders will be ok. But we'll wait until the fontsize ticket is merged before fixing it, if possible.
Apart from that and the comments below, the only thing I don't like is that the icon for the list of poll and the icon for the public poll are the same. These are very different pages that deserve different icons.
| @@ -63,3 +79,11 @@ nav | |||
| background-color: color.adjust($maincolor, $lightness: 10%) | |||
| a.active:hover | |||
| background-color: $nav-hover-bg | |||
|
|
|||
| hr | |||
There was a problem hiding this comment.
Please be more precise. Say at least "hr in mat-menu".
Ticket #76
| <button *ngIf="(demoIsActive$ | async) && !(session.state$ | async).logged" mat-menu-item class="nav-link"><mat-icon>play_circle_outline</mat-icon> | ||
| <a [routerLink]="(demoPath$ | async)" routerLinkActive="active"> | ||
| <span class="navitem" i18n>Try it</span> | ||
| </a> |
There was a problem hiding this comment.
Both the <a> and the <span> elements are useless here. You could move the routerLink directives into the button element. Of course, this applies to the other two menu items.
| mat-menu .nav-link | ||
| @media (min-width: $breakpoint-tablet) | ||
| display: none |
There was a problem hiding this comment.
This does not work. The content of the menu is moved from the mat-menu to the overlay (out of reach) when the user opens the menu. Hence it is not possible to have CSS selectors that "cross" the menu limit. From my experiment, the solution here is to add a div in the menu with an id, and to use this id to detect that the .nav-link is in the menu.
app/src/styles.sass
Outdated
| @media (min-width: $breakpoint-tablet) | ||
| display: none |
There was a problem hiding this comment.
This must not be here, in the global style file. We do want all the hr in all menus to look the same. But we do not want all the hr in all menus to disappear when the screen is wide. This has to be moved in navtitle.component.sass.
|
PTAL |
| </button> | ||
| <button mat-menu-item (click)="logoff()"><mat-icon>logout</mat-icon> Log off</button> | ||
| <div id="menubutton"> | ||
| <button *ngIf="(demoIsActive$ | async) && !(session.state$ | async).logged" mat-menu-item class="nav-link" [routerLink]="(demoPath$ | async)" routerLinkActive="active"> |
There was a problem hiding this comment.
This line is much too long (172 characters). I try to respect a soft limit of 100 and a hard limit of 120. Considering this hard limit, lines 12, 15, 35, 39, 43 and 47 are too long too.
| <button *ngIf="(demoIsActive$ | async) && !(session.state$ | async).logged" mat-menu-item class="nav-link" [routerLink]="(demoPath$ | async)" routerLinkActive="active"> | |
| <button *ngIf="(demoIsActive$ | async) && !(session.state$ | async).logged" mat-menu-item class="nav-link" | |
| [routerLink]="(demoPath$ | async)" routerLinkActive="active"> |
| <span class="navitem">Create</span> | ||
| </a> | ||
| <a *ngIf="(session.state$ | async).logged" [matMenuTriggerFor]="profileMenu"> | ||
| <a [matMenuTriggerFor]="profileMenu" id="naviconmenu"> |
There was a problem hiding this comment.
Names are easier to read and remember if the words are separated. With CSS, the usage is to separate words using -. Hence naviconmenu would be more readable if it was nav-icon-menu. Same for nav-icon-person below. I haven't done that for all class and id names and I regret it. Notice too that the elements on which these id are applied are not icons, which can be misleading.
| </ng-container> Notifications | ||
| </button> | ||
| <button mat-menu-item (click)="logoff()"><mat-icon>logout</mat-icon> Log off</button> | ||
| <div id="menubutton"> |
There was a problem hiding this comment.
This is not a button. Perhaps menu-buttons (mind the plural). You could also reuse mat-menu here, because classes and ids have different namespaces.
| @@ -63,3 +79,11 @@ nav | |||
| background-color: color.adjust($maincolor, $lightness: 10%) | |||
| a.active:hover | |||
| background-color: $nav-hover-bg | |||
|
|
|||
| hr | |||
|
PTAL |
Ticket #76 : Put the different options of the title bar in a drop-down menu.