Conversation
There was a problem hiding this comment.
Pull request overview
This pull request converts SVG assets to GTS (Glimmer TypeScript) components and refactors the codebase to use component-based icons instead of static SVG files. The PR also modernizes several UI components by replacing the ember-click-outside library with the native Popover API and CSS Anchor Positioning.
Changes:
- Converts 20+ SVG files to reusable GTS icon components across three packages (ember-ui, ember-input, ember-input-validation)
- Refactors actions menu, navbar, and introduces a new theme selector component to use the Popover API
- Updates type signatures to accept icon components instead of string paths
- Removes static SVG assets and updates package.json configurations accordingly
- Adds pre-commit linting hook and updates translations
Reviewed changes
Copilot reviewed 62 out of 78 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ember-ui/src/assets/icons/*.gts | New icon components created from SVG files |
| packages/ember-ui/src/components/tpk-actions-menu.gts | Refactored to use Popover API with CSS anchor positioning |
| packages/ember-ui/src/components/prefabs/tpk-theme-selector.gts | New theme selector component using Popover API |
| packages/ember-ui/src/components/prefabs/tpk-navbar.gts | Updated to use Popover API for dropdowns and icon components |
| packages/ember-ui/src/components/tpk-table-generic/* | Updated to accept icon components instead of paths |
| packages/ember-input/src/components/prefabs/tpk-search.gts | Refactored to use SearchIcon component |
| packages/ember-input-validation/src/components/prefabs/*.gts | Updated validation components to use icon components |
| packages/ember-ui/package.json | Removed public-assets configuration for deleted SVGs |
| packages/ember-input-validation/package.json | Removed public-assets configuration for deleted SVGs |
| doc-app/tests/integration/components/ember-ui/*.gts | Updated tests for Popover API and icon components |
| doc-app/tests/pages/ember-actions-menu.ts | Added popover state detection helper |
| lefthook.yml | Added pre-commit hook for automatic linting |
| doc-app/translations/*.yaml | Added theme and search translations |
Comments suppressed due to low confidence (3)
doc-app/tests/integration/components/ember-ui/action-menu-test.gts:59
- The test for keyboard interaction (pressing ESC to close the menu) was removed. With the popover API, the browser natively handles ESC to close popovers, but it would be good to have a test verifying this behavior still works. Consider adding a test that validates the ESC key closes the popover to ensure the native popover behavior is working as expected.
test('All base classes are present', async function (assert) {
await renderActionMenu(assert);
assert.dom('.actions').exists();
assert.dom('.open_actions').exists();
});
});
doc-app/tests/integration/components/ember-ui/prefabs/tpk-table-generic-prefab-test.gts:108
- The 'icon' properties were removed from the actionMenu test data, but the test doesn't verify that the menu still works correctly without icons. While the icon was made optional in the type definition, it would be good to have a test that explicitly verifies both cases: with and without icons.
const actionMenu = [
{
action: () => {
assert.step('rowClick function called');
},
name: 'Edit',
},
{
action: () => {
assert.step('delete function called');
},
name: 'Delete',
},
packages/ember-input/src/components/prefabs/tpk-search.css:36
- The loader animation uses a hardcoded color '#333' which may not work well with different theme backgrounds. The previous version used 'white' which was also hardcoded but has been removed. Consider using a CSS variable or currentColor to ensure the loader is visible across different themes.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
doc-app/tests/integration/components/ember-ui/prefabs/tpk-table-generic-prefab-test.gts
Show resolved
Hide resolved
…VG in navbar with ChevronDownIcon
No description provided.