feat(grid): provide grid keyboard navigation directives #1400
Conversation
Summary of ChangesHello @spliffone, 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 introduces a robust and flexible solution for keyboard navigation within grid-like structures in Angular applications. By leveraging a rectangle-based navigation service, it enables seamless keyboard interaction across diverse CSS layouts, including CSS Grid and Flexbox. The new directives provide comprehensive ARIA support and implement a roving tabindex pattern, significantly enhancing the accessibility of grid components. 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 a new set of directives for creating accessible grids with keyboard navigation. The implementation is robust, leveraging rectangle-based positioning for layout independence, which is a great approach. The code is well-structured and uses modern Angular signals effectively. I've left a few comments with suggestions for minor improvements, mainly around code duplication, documentation accuracy, and improving maintainability by removing magic numbers. Overall, this is a solid contribution.
| private calculateDistance(from: DOMRect, to: DOMRect, direction: Direction): number { | ||
| if (direction === 'right' || direction === 'left') { | ||
| // Horizontal movement: prioritize vertical alignment | ||
| const verticalOverlap = this.calculateOverlap(from.top, from.bottom, to.top, to.bottom); | ||
| const horizontalDist = Math.abs( | ||
| direction === 'right' ? to.left - from.right : from.left - to.right | ||
| ); | ||
|
|
||
| // If well-aligned (>50% overlap), use horizontal distance | ||
| // Otherwise, heavily penalize misalignment | ||
| if (verticalOverlap > 0.5) { | ||
| return horizontalDist; | ||
| } else { | ||
| // Add penalty based on vertical misalignment | ||
| const verticalMisalignment = Math.abs( | ||
| (from.top + from.bottom) / 2 - (to.top + to.bottom) / 2 | ||
| ); | ||
| return horizontalDist * 2 + verticalMisalignment * 10; | ||
| } | ||
| } else { | ||
| // Vertical movement: prioritize horizontal alignment | ||
| const horizontalOverlap = this.calculateOverlap(from.left, from.right, to.left, to.right); | ||
| const verticalDist = Math.abs( | ||
| direction === 'down' ? to.top - from.bottom : from.top - to.bottom | ||
| ); | ||
|
|
||
| // If well-aligned (>50% overlap), use vertical distance | ||
| // Otherwise, heavily penalize misalignment | ||
| if (horizontalOverlap > 0.5) { | ||
| return verticalDist; | ||
| } else { | ||
| // Add penalty based on horizontal misalignment | ||
| const horizontalMisalignment = Math.abs( | ||
| (from.left + from.right) / 2 - (to.left + to.right) / 2 | ||
| ); | ||
| return verticalDist * 2 + horizontalMisalignment * 10; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The calculateDistance method uses several magic numbers (0.5, 2, 10) for its heuristics. To improve readability and maintainability, consider extracting these values into named constants at the class level, for example ALIGNMENT_THRESHOLD, DISTANCE_FACTOR, and MISALIGNMENT_PENALTY.
| * Get all cells regardless of mode (explicit rows or flat). | ||
| * @internal | ||
| */ |
There was a problem hiding this comment.
The comment here is a bit misleading. It states that it gets cells 'regardless of mode (explicit rows or flat)', but the current implementation only supports the flat mode by using directCells. It would be clearer to update the comment to reflect the current implementation, for example: 'Get all cells for a flat grid layout.'
| readonly derivedRows = computed<DerivedRow<SiGridCellDirective>[]>(() => { | ||
| // Derive rows from cell positions | ||
| const cells = this.directCells(); | ||
| const rowMap = new Map<number, SiGridCellDirective[]>(); | ||
|
|
||
| cells.forEach(cell => { | ||
| const rect = cell.getBoundingRect(); | ||
| const rowKey = Math.round(rect.top); | ||
|
|
||
| if (!rowMap.has(rowKey)) { | ||
| rowMap.set(rowKey, []); | ||
| } | ||
| rowMap.get(rowKey)!.push(cell); | ||
| }); | ||
|
|
||
| // Sort rows by Y, cells within rows by X | ||
| return Array.from(rowMap.entries()) | ||
| .sort(([y1], [y2]) => y1 - y2) | ||
| .map(([yPosition, rowCells], index) => ({ | ||
| cells: rowCells.sort((a, b) => a.getBoundingRect().left - b.getBoundingRect().left), | ||
| yPosition, | ||
| rowIndex: index + 1 | ||
| })); | ||
| }); |
There was a problem hiding this comment.
The logic to derive rows from cell positions is duplicated here and in the private groupCellsByRows method within SiGridNavigationService. To improve maintainability and centralize grid calculation logic, consider extracting this functionality into a public method in SiGridNavigationService. This new method could be used by both the directive and the service's internal methods.
|
|
|
Documentation. Coverage Reports: |
See #1345