Conversation
Summary of ChangesHello @dauriamarco, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience of the launchpad component by introducing spatial keyboard navigation, making it more accessible and intuitive. It also refines the launchpad's visual design and behavior, incorporating new animations and flexible configuration options for displaying app categories and favorites. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces spatial arrow key navigation for the launchpad, which is a significant accessibility enhancement. The implementation is well-structured, leveraging modern Angular features and including a visual refresh with new animations. I've identified a critical bug in the navigation logic that affects layouts with single-item rows, and I've provided a fix. Additionally, I've included suggestions to improve test clarity and coverage.
| // Group apps by rows (excluding current app for target selection) | ||
| const rowGroups = new Map<number, typeof appRects>(); | ||
| let hasMultipleRows = false; | ||
|
|
||
| appRects.forEach((appRect, index) => { | ||
| const rowKey = Math.round(appRect.rect.top / this.rowTolerance) * this.rowTolerance; | ||
|
|
||
| if (rowKey !== currentRowKey) { | ||
| hasMultipleRows = true; | ||
| } | ||
|
|
||
| if (index !== currentIndex) { | ||
| if (!rowGroups.has(rowKey)) { | ||
| rowGroups.set(rowKey, []); | ||
| } | ||
| rowGroups.get(rowKey)!.push(appRect); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The logic for grouping apps into rows has a flaw. By excluding the current app with if (index !== currentIndex), you introduce a bug where if an app is the only one in its row, its row key is not added to rowGroups. This causes currentRowIndex to become -1, leading to incorrect vertical navigation (e.g., pressing down moves to the top row instead of the next row).
The fix is to include all apps when building rowGroups. This makes the logic for finding the current and target rows more robust. The comment "excluding current app for target selection" is also misleading, as the current app should be part of the row grouping to correctly identify the current row's index.
// Group apps by rows
const rowGroups = new Map<number, typeof appRects>();
let hasMultipleRows = false;
appRects.forEach(appRect => {
const rowKey = Math.round(appRect.rect.top / this.rowTolerance) * this.rowTolerance;
if (rowKey !== currentRowKey) {
hasMultipleRows = true;
}
if (!rowGroups.has(rowKey)) {
rowGroups.set(rowKey, []);
}
rowGroups.get(rowKey)!.push(appRect);
});| await page.getByRole('link', { name: 'Fischbach' }).first().locator('.favorite-icon').click(); | ||
| await si.runVisualAndA11yTests('new favorite'); |
There was a problem hiding this comment.
The test case now verifies un-favoriting an app, but the corresponding visual snapshot is named 'new favorite'. This is misleading and can cause confusion. To improve clarity, I recommend either updating the test to favorite an app that isn't already a favorite (e.g., one of the 'App name' apps) or renaming the snapshot to more accurately reflect the action being tested, such as 'unfavorite-app'.
| describe('tolerance handling', () => { | ||
| it('should detect single column within leftTolerance (20px)', () => { | ||
| const mockApps = createMockApps([ | ||
| { top: 0, left: 10 }, | ||
| { top: 80, left: 25 }, | ||
| { top: 160, left: 15 } | ||
| ]); | ||
|
|
||
| const result = (component as any).getVerticalTargetIndex(mockApps, 1, false); | ||
| expect(result).toBe(2); // Sequential navigation | ||
| }); | ||
|
|
||
| it('should detect same row within rowTolerance (10px)', () => { | ||
| const mockApps = createMockApps([ | ||
| { top: 50, left: 10 }, | ||
| { top: 50, left: 130 }, | ||
| { top: 50, left: 250 } | ||
| ]); | ||
|
|
||
| const result = (component as any).getVerticalTargetIndex(mockApps, 1, false); | ||
| expect(result).toBe(1); // No movement | ||
| }); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The tests for keyboard navigation are a great addition. However, they miss an important edge case that would have caught the bug in the navigation logic: a grid layout where one of the rows contains only a single item. Adding a test for this scenario will make the test suite more robust and prevent future regressions.
describe('uneven grid layout', () => {
it('should handle rows with a single item correctly', () => {
const mockApps = createMockApps([
{ top: 50, left: 10 }, { top: 50, left: 130 }, // Row 0
{ top: 130, left: 70 }, // Row 1 (single item)
{ top: 210, left: 10 }, { top: 210, left: 130 } // Row 2
]);
// Navigate down from row 1 to row 2
expect((component as any).getVerticalTargetIndex(mockApps, 2, false)).toBe(3);
// Navigate up from row 1 to row 0
expect((component as any).getVerticalTargetIndex(mockApps, 2, true)).toBe(1);
});
});
});
});
|
Documentation. Coverage Reports: |
Align the launchpad to match iX/Element styling, improve responsiveness, animations and accessibility. BREAKING CHANGE: removed default subtitle text from launchpad The `subtitleText` input no longer shows "Access all your apps" by default. To maintain the previous behavior, explicitly set the input.
dauriamarco
force-pushed
the
feat/spatial-arrow-navigation
branch
from
c5ca59a to
7262aec
Compare
|
|
1 participant
Closes #1345. Starting point to introduce a general directive to enable spatial grid navigation by arrow keys.