Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughTwo separate component updates are applied. The branding component now imports Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR makes small UI/template adjustments in the Angular frontend, primarily improving hover tooltips in the DB tables list and ensuring routing directives are available in the Branding standalone component.
Changes:
- Refactors the collapsed tables list markup and adjusts where tooltips are applied.
- Adds tooltips (with length-based disabling) for folder names and table entries in the expanded tables list.
- Imports
RouterModuleintoBrandingComponentto supportrouterLinkusage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontend/src/app/components/dashboard/db-tables-list/db-tables-list.component.html | Updates collapsed/expanded table list templates and adds tooltips for names. |
| frontend/src/app/components/branding/branding.component.ts | Adds RouterModule to standalone component imports for routerLink. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div *ngFor="let tableItem of getCollapsedTableList()" | ||
| [matTooltip]="getTableName(tableItem)" | ||
| matTooltipPosition="right" | ||
| (click)="closeSidebar()"> | ||
| <!-- Show icon if available, otherwise show initials --> | ||
| <mat-icon *ngIf="tableItem.icon; else tableNameInitials" class="table-list-icon"> | ||
| {{tableItem.icon}} | ||
| </mat-icon> | ||
| <ng-template #tableNameInitials> | ||
| {{tableItem.initials}} | ||
| </ng-template> | ||
| </a> | ||
| matTooltipPosition="right"> | ||
| <a class="collapsed-table-item" |
There was a problem hiding this comment.
matTooltip is attached to the wrapping <div> (not the <a>), but the <div> is not focusable. This means keyboard users focusing the link won’t get the tooltip on focus (regression vs when tooltip was on the anchor). Move the tooltip bindings onto the <a> or make the tooltip host focusable (e.g., tabindex="0") and ensure focus/aria behavior is correct.
| <div *ngFor="let table of getFolderTables(folder)" | ||
| class="selected-table-item" | ||
| [matTooltip]="getTableName(table)" | ||
| [matTooltipDisabled]="getTableNameLength(getTableName(table)) <= 18" | ||
| cdkDrag |
There was a problem hiding this comment.
The tooltip is placed on .selected-table-item (a <div> without tabindex), while the interactive element is the inner <a>. As a result, the tooltip won’t appear when navigating via keyboard focus on the link. Consider moving matTooltip/matTooltipDisabled onto the <a> (or the .table-name span) so it triggers on focus as well as hover.
| [matTooltip]="getTableName(table)" | ||
| [matTooltipDisabled]="getTableNameLength(getTableName(table)) <= 18" | ||
| cdkDrag |
There was a problem hiding this comment.
Within the *ngFor this introduces multiple method calls per row (getTableName(table) is invoked for matTooltip, again inside matTooltipDisabled, and again in the link text). This can add noticeable change-detection cost for large lists. Prefer computing the value once (e.g., via *ngIf="getTableName(table) as tableName" and reusing tableName, or by precomputing a tableName field in the data).
Summary by CodeRabbit