icons tidy#149
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 35 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a shared SVG icon primitive and migrates header controls, generator controls, instruction rendering, help icons, CSS, and tests to render and consume accessible inline SVG icons with updated button markup and Playwright selector scoping. ChangesIcon Standardization Migration
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes UI icons across core grid/editor/generator/help surfaces by introducing an app-owned inline-SVG icon primitive (Lucide-style) and migrating existing ad hoc text/Unicode/CSS/Font Awesome-based icons to the shared helper, with accompanying accessibility-focused test updates.
Changes:
- Added a shared
iconprimitive that renders an allowlisted set of inline SVG icons as DOM nodes or HTML strings. - Replaced legacy header/help/schema/generator icons with the shared SVG approach and updated CSS to style the new icon system.
- Updated Jest + Playwright tests (and page abstractions) to assert on new icon markup and accessible labels; removed the legacy Font Awesome SVG asset.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core-ui/src/tests/utils/help-tooltips.test.js | Updates tooltip tests to assert new inline-SVG help icon rendering and tippy-unavailable fallback. |
| packages/core-ui/src/tests/shared/instructions-view.test.js | Asserts instruction items now render standardized icons with expected selectors/titles. |
| packages/core-ui/src/tests/shared/icon-primitive.test.js | Adds unit tests for the new shared icon primitive (DOM + HTML rendering, idempotence, unknown icon errors). |
| packages/core-ui/src/tests/grid/tabulator-custom-header.test.js | Updates Tabulator header popup tests for button-based icon controls and accessibility attributes. |
| packages/core-ui/src/tests/grid/data-grid-component-view.test.js | Adds assertions for standardized header icon markup and accessible labels in generated header HTML. |
| packages/core-ui/src/tests/grid/ag-grid-header-components.test.js | Updates AG Grid header tests to validate new button + SVG icon markup. |
| packages/core-ui/src/tests/generator/generator-controls-view.test.js | Updates generator controls tests to assert the standardized file icon SVGs exist. |
| packages/core-ui/src/tests/generator/data-generator-schema-ui.test.js | Adds assertions for schema row action aria-labels and icon SVG presence. |
| packages/core-ui/js/help/help-tooltips.js | Ensures help icons render even without tippy by decorating containers with shared SVG icons before attaching tooltips. |
| packages/core-ui/js/gui_components/shared/primitives/icon/index.js | Introduces shared allowlisted inline-SVG icon rendering utilities (DOM + HTML string). |
| packages/core-ui/js/gui_components/shared/instructions/instructions-view.js | Renders instruction list items with standardized icons via renderIconHtml. |
| packages/core-ui/js/gui_components/shared/instructions/app-page-instructions.js | Converts app instruction items from legacy glyphs to {icon,title,text} objects aligned with the new icon system. |
| packages/core-ui/js/gui_components/generator/schema/data-generator-schema-ui.js | Replaces schema row glyphs with standardized inline SVG icons and adds aria-labels/type=button. |
| packages/core-ui/js/gui_components/generator/controls/generator-controls-view.js | Replaces CSS-drawn file icon with standardized file-plus inline SVG icon. |
| packages/core-ui/js/gui_components/data-grid-editor/tabulator/customHeader-tabulator.js | Replaces popup header action glyphs with button-based SVG icon controls. |
| packages/core-ui/js/gui_components/data-grid-editor/data-grid-component-view.js | Replaces Tabulator header icons with standardized SVGs and adds aria-label/title attributes to action elements. |
| packages/core-ui/js/gui_components/data-grid-editor/ag-grid/customHeader-ag-grid.js | Replaces AG Grid header action glyphs with standardized SVG icon buttons (keeps AG Grid chrome icons). |
| docs/icon-standardization-plan.md | Documents the icon standardization goals, mapping, and verification approach. |
| apps/web/styles.css | Adds shared icon/button styling and updates help/generator/header icon styles; removes legacy help-icon background image usage. |
| apps/web/src/tests/browser/app/functional/grid-editor/column-operations/add-columns.spec.js | Updates Playwright assertions to locate header actions via aria-label instead of legacy title. |
| apps/web/src/tests/browser/app/abstractions/components/grid-header.component.js | Updates header action lookup logic to prefer aria-label (fallback to title). |
| apps/web/images/readme.md | Removes obsolete documentation referencing the old Font Awesome SVG. |
| apps/web/images/circle-question-solid.svg | Removes the legacy Font Awesome help icon asset. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/icon-standardization-plan.md (1)
16-20: 💤 Low valueConsider varying sentence structure for readability.
The repeated "Do not" beginnings make the constraints clear but could be varied for better flow. This is purely a style suggestion.
📝 Optional style improvement
## Non-Goals -- Do not redesign grid behavior, sorting, filtering, or column editing while replacing icons. -- Do not replace Tabulator or AG Grid as part of this work. -- Do not remove visible text from primary text buttons such as `Add Row` unless the button is intentionally converted to an icon-only control with accessible labeling. -- Do not rewrite existing components solely to fit the icon work unless a local component boundary is already being touched. -- Do not replace Tippy itself; this plan keeps the existing tooltip service contract. +- Avoid redesigning grid behavior, sorting, filtering, or column editing while replacing icons. +- Tabulator and AG Grid replacement are out of scope for this work. +- Preserve visible text from primary text buttons such as `Add Row` unless the button is intentionally converted to an icon-only control with accessible labeling. +- Avoid rewriting existing components solely to fit the icon work unless a local component boundary is already being touched. +- Keep the existing Tippy tooltip service contract; replacement is not in scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/icon-standardization-plan.md` around lines 16 - 20, Revise the repeated "Do not" sentence openings in the constraints list (e.g., the bullets starting "Do not redesign grid behavior, sorting, filtering, or column editing...", "Do not replace Tabulator or AG Grid...", "Do not remove visible text from primary text buttons such as `Add Row`...", "Do not rewrite existing components solely to fit the icon work...", and "Do not replace Tippy itself; this plan keeps the existing tooltip service contract.") to vary sentence structure for improved readability—rewrite some items to use affirmative or mixed phrasing (e.g., "Keep grid behavior, sorting, filtering, and column editing unchanged" or "Preserve Tabulator and AG Grid") while preserving the same constraints and meaning.packages/core-ui/js/gui_components/generator/schema/data-generator-schema-ui.js (1)
339-349: ⚡ Quick winHarden generator row action click handler against nested icon targets
handleGeneratorRowButtonClick(lines 339-349) readsevent.target.getAttribute('data-action'/'data-row-id'). Today this is likely safe because the SVGs come fromrenderIconHtml()with the defaultapp-iconclass, andapps/web/styles.csssets.app-icon { pointer-events: none; }, so clicks land on the<button>.Still fragile: make the handler resolve the actual button via
closest('[data-action]')(same robustness pattern used for drag).Proposed change
function handleGeneratorRowButtonClick({ event, schemaRows, addRowAfter, removeRow, moveRow }) { - const action = event.target.getAttribute('data-action'); + const actionButton = event.target?.closest?.('[data-action]'); + const action = actionButton?.getAttribute('data-action'); if (!action || action === 'drag') { return; } - const rowId = event.target.getAttribute('data-row-id'); + const rowId = actionButton.getAttribute('data-row-id'); const index = schemaRows.findIndex((row) => row.id === rowId); if (index < 0) { return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core-ui/js/gui_components/generator/schema/data-generator-schema-ui.js` around lines 339 - 349, The click handler handleGeneratorRowButtonClick is fragile because it reads attributes from event.target which can be a nested SVG or icon; change it to find the actual button element first (e.g., const btn = event.target.closest('[data-action]') or similar) and then read btn.getAttribute('data-action') and btn.getAttribute('data-row-id'); if closest returns null keep the existing early return logic and continue using schemaRows.findIndex(...). This mirrors the robustness used for drag handling and prevents missing attributes when icons are clicked.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/styles.css`:
- Line 236: Several CSS declarations use the keyword "currentColor" with
uppercase C, which triggers the stylelint value-keyword-case rule; update each
occurrence (e.g., the declarations currently written as "color: currentColor;"
at the spots you added) to use the lowercase keyword "currentcolor" so they
conform to the project's linting convention; search for the exact token
"currentColor" in the stylesheet and replace with "currentcolor" across all
occurrences (including the ones referenced at lines around 236, 380, 463–464,
and 554).
In `@packages/core-ui/js/gui_components/shared/instructions/instructions-view.js`:
- Around line 26-33: The template's call to renderIconHtml using item.icon can
throw and abort rendering; update the template() logic around iconName/item.icon
to fail soft by validating or try/catching renderIconHtml (the code that sets
iconHtml) so that on an unknown icon you set iconHtml to an empty string (i.e.
render text-only) instead of letting renderIconHtml throw; reference
renderIconHtml, iconName/item.icon and the instruction-item-icon HTML generation
when making the change.
---
Nitpick comments:
In `@docs/icon-standardization-plan.md`:
- Around line 16-20: Revise the repeated "Do not" sentence openings in the
constraints list (e.g., the bullets starting "Do not redesign grid behavior,
sorting, filtering, or column editing...", "Do not replace Tabulator or AG
Grid...", "Do not remove visible text from primary text buttons such as `Add
Row`...", "Do not rewrite existing components solely to fit the icon work...",
and "Do not replace Tippy itself; this plan keeps the existing tooltip service
contract.") to vary sentence structure for improved readability—rewrite some
items to use affirmative or mixed phrasing (e.g., "Keep grid behavior, sorting,
filtering, and column editing unchanged" or "Preserve Tabulator and AG Grid")
while preserving the same constraints and meaning.
In
`@packages/core-ui/js/gui_components/generator/schema/data-generator-schema-ui.js`:
- Around line 339-349: The click handler handleGeneratorRowButtonClick is
fragile because it reads attributes from event.target which can be a nested SVG
or icon; change it to find the actual button element first (e.g., const btn =
event.target.closest('[data-action]') or similar) and then read
btn.getAttribute('data-action') and btn.getAttribute('data-row-id'); if closest
returns null keep the existing early return logic and continue using
schemaRows.findIndex(...). This mirrors the robustness used for drag handling
and prevents missing attributes when icons are clicked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c6a71dbe-72a5-43f2-b211-78e451cea223
⛔ Files ignored due to path filters (1)
apps/web/images/circle-question-solid.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
apps/web/images/readme.mdapps/web/src/tests/browser/app/abstractions/components/grid-header.component.jsapps/web/src/tests/browser/app/functional/grid-editor/column-operations/add-columns.spec.jsapps/web/styles.cssdocs/icon-standardization-plan.mdpackages/core-ui/js/gui_components/data-grid-editor/ag-grid/customHeader-ag-grid.jspackages/core-ui/js/gui_components/data-grid-editor/data-grid-component-view.jspackages/core-ui/js/gui_components/data-grid-editor/tabulator/customHeader-tabulator.jspackages/core-ui/js/gui_components/generator/controls/generator-controls-view.jspackages/core-ui/js/gui_components/generator/schema/data-generator-schema-ui.jspackages/core-ui/js/gui_components/shared/instructions/app-page-instructions.jspackages/core-ui/js/gui_components/shared/instructions/instructions-view.jspackages/core-ui/js/gui_components/shared/primitives/icon/index.jspackages/core-ui/js/help/help-tooltips.jspackages/core-ui/src/tests/generator/data-generator-schema-ui.test.jspackages/core-ui/src/tests/generator/generator-controls-view.test.jspackages/core-ui/src/tests/grid/ag-grid-header-components.test.jspackages/core-ui/src/tests/grid/data-grid-component-view.test.jspackages/core-ui/src/tests/grid/tabulator-custom-header.test.jspackages/core-ui/src/tests/shared/icon-primitive.test.jspackages/core-ui/src/tests/shared/instructions-view.test.jspackages/core-ui/src/tests/utils/help-tooltips.test.js
💤 Files with no reviewable changes (1)
- apps/web/images/readme.md
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/tests/browser/app/abstractions/components/grid-editor.component.js (1)
21-21: 💤 Low valueUpdate checkbox selector to an accessible contract
id="uniqueColumnNamesCheckbox"exists, but the checkbox is wrapped by a<label>with the text “Unique Column Names”, so it has an accessible name. Preferthis.toolbar.getByRole('checkbox', { name: 'Unique Column Names' })(orgetByLabel('Unique Column Names')) instead oflocator('#uniqueColumnNamesCheckbox')to match the rest of the abstraction and avoid DOM-internal IDs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/tests/browser/app/abstractions/components/grid-editor.component.js` at line 21, Replace the direct ID-based locator for the checkbox with an accessible query to follow the abstraction contract: update the assignment to use this.toolbar.getByRole('checkbox', { name: 'Unique Column Names' }) (or this.toolbar.getByLabel('Unique Column Names')) instead of this.toolbar.locator('`#uniqueColumnNamesCheckbox`') so the component uses the accessible name and avoids depending on internal DOM IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/web/src/tests/browser/app/abstractions/components/grid-editor.component.js`:
- Line 21: Replace the direct ID-based locator for the checkbox with an
accessible query to follow the abstraction contract: update the assignment to
use this.toolbar.getByRole('checkbox', { name: 'Unique Column Names' }) (or
this.toolbar.getByLabel('Unique Column Names')) instead of
this.toolbar.locator('`#uniqueColumnNamesCheckbox`') so the component uses the
accessible name and avoids depending on internal DOM IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 93bf309d-6ec0-4acc-90b2-40519b9d7dbc
📒 Files selected for processing (9)
apps/web/images/.gitkeepapps/web/src/tests/browser/app/abstractions/components/grid-editor.component.jsapps/web/styles.csspackages/core-ui/js/gui_components/data-grid-editor/data-grid-component-view.jspackages/core-ui/js/gui_components/generator/schema/data-generator-schema-ui.jspackages/core-ui/js/gui_components/shared/instructions/instructions-view.jspackages/core-ui/src/tests/generator/data-generator-schema-ui.test.jspackages/core-ui/src/tests/grid/data-grid-component-view.test.jspackages/core-ui/src/tests/shared/instructions-view.test.js
✅ Files skipped from review due to trivial changes (1)
- apps/web/images/.gitkeep
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core-ui/src/tests/grid/data-grid-component-view.test.js
- packages/core-ui/js/gui_components/shared/instructions/instructions-view.js
- apps/web/styles.css
- packages/core-ui/js/gui_components/data-grid-editor/data-grid-component-view.js
- packages/core-ui/js/gui_components/generator/schema/data-generator-schema-ui.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
amend icons to use a consistent framework
Summary by CodeRabbit
New Features
Style
Documentation
Tests