feat(core): refactor keyboard navigation #2268
Conversation
🦋 Changeset detectedLatest commit: df279b0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello @danielleroux, 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 significant overhaul of keyboard navigation and focus management for 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 refactors the keyboard navigation for dropdown-related components to align with W3C accessibility patterns. It introduces a new focus management system using an ix-focusable class and a global listener to apply ix-focused styles, replacing the previous ArrowFocusController. A new showChange event is added to dropdowns to allow cancellation before showing. The changes are extensive and touch upon core components, their styling, and framework-specific wrappers. My review focuses on potential issues like the use of !important in styles, an empty catch block that could hide errors, leftover debug code, and unclear documentation.
| } | ||
|
|
||
| /* eslint-disable-next-line */ | ||
| } catch {} |
There was a problem hiding this comment.
The catch block is empty. This will swallow any errors that occur within the try block, making debugging difficult. It's better to at least log the error to the console, especially during development.
| } catch {} | |
| } catch (e) { console.error(e); } |
There was a problem hiding this comment.
@gemini-code-assist I do not agree, i do not want that the user see any error message in the console
There was a problem hiding this comment.
I understand your concern about not wanting end-users to see error messages in the console. However, an empty catch block can make debugging very challenging, as it completely hides any issues that might occur during the DOM querying process.
To address this, I recommend logging errors conditionally. This approach ensures that errors are visible to developers during testing and development, which is crucial for identifying and fixing issues, without exposing potentially sensitive or confusing error messages to end-users in a production environment.
This can be achieved by checking the NODE_ENV environment variable:
/* eslint-disable-next-line */
} catch (e) {
if (process.env.NODE_ENV === 'development') {
console.error('Error during dropdown item collection:', e);
}
// Optionally, you could send errors to an internal logging service in production here.
}Silently swallowing errors can lead to unexpected behavior in the UI that is very difficult to trace back to its origin. Even if not visible to the end-user, having a record of these errors is crucial for maintaining the stability and reliability of the component.
| } catch {} | |
| /* eslint-disable-next-line */ | |
| } catch (e) { | |
| if (process.env.NODE_ENV === 'development') { | |
| console.error('Error during dropdown item collection:', e); | |
| } | |
| } |
| */ | ||
| showChange: EventEmitter<CustomEvent<boolean>>; | ||
| /** | ||
| * Fire event after visibility of dropdown has changed, preventing event will prevent nothing |
There was a problem hiding this comment.
The phrase "preventing event will prevent nothing" is a bit confusing. It could be rephrased for better clarity, for example: "This event is not preventable." or "Calling preventDefault() on this event has no effect." This comment also applies to other generated files where this documentation appears.
| * Fire event after visibility of dropdown has changed, preventing event will prevent nothing | |
| * Fire event after visibility of dropdown has changed. This event is not preventable. |
| outline: 1px solid var(--theme-color-focus-bdr) !important; | ||
| outline-offset: var(--theme-btn--focus--outline-offset) !important; |
There was a problem hiding this comment.
The use of !important can make styles difficult to override and debug. Is it possible to achieve the desired focus styling by increasing specificity or reordering CSS rules to avoid using !important?
| outline: 1px solid var(--theme-color-focus-bdr) !important; | ||
| outline-offset: var(--theme-btn--focus--outline-offset) !important; |
There was a problem hiding this comment.
The use of !important here can make these styles hard to override. Could this be avoided by using higher specificity selectors or a different approach?
| if (this.assignedSubmenu.indexOf(detail) === -1) { | ||
| this.assignedSubmenu.push(detail); | ||
| } | ||
| console.log('parent', this.localUId, 'child', detail); |
There was a problem hiding this comment.
There's a console.log statement here. This seems like debug code that should be removed before merging.
|
💡 What is the current behavior?
GitHub Issue Number: #
🆕 What is the new behavior?
Refactor keyboard navigation to match the common w3 accessibility patterns for
ix-dropdownrelated components🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support