fix(core/aggrid): updated row zebra pattern - #2638
Conversation
✅ Deploy Preview for ix-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
📝 WalkthroughWalkthroughThe PR updates AG Grid theme color parameters, adds hover row-state styling to the IX theme, and introduces a Storybook AG Grid example with community modules, sample data, themed rendering, selection, tooltips, autosizing, and draggable rows. ChangesAG Grid theme and Storybook example
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Storybook
participant renderGrid
participant createIxTheme
participant createGrid
Storybook->>renderGrid: render Default story
renderGrid->>createIxTheme: create IX theme
renderGrid->>createGrid: pass grid options, row data, and theme
createGrid-->>Storybook: render configured AG Grid
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
GayatriK2002
changed the title
![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 updates AG Grid theme parameters and introduces a new row-state stylesheet to paint the row-hover tint directly on cells, resolving zebra background inconsistencies. Feedback suggests using an inset box-shadow instead of background-image to avoid overriding custom cell background images, and setting AG Grid's built-in rowHoverColor to transparent to prevent double-hover tinting.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| .ag-row.ag-row-hover .ag-cell { | ||
| background-image: linear-gradient( | ||
| var(--theme-color-ghost--hover), | ||
| var(--theme-color-ghost--hover) | ||
| ) !important; | ||
| } |
There was a problem hiding this comment.
Using background-image: linear-gradient(...) !important to apply the hover overlay will completely override any custom background-image (such as icons, custom gradients, or patterns) that might be set on cells by custom cell renderers or other styles.
To avoid overriding cell background images while still achieving the desired overlay effect, you can use an inset box-shadow instead. An inset box-shadow overlays on top of both background-color and background-image without replacing them.
Additionally, since we are manually painting the hover overlay on the cell, please ensure that AG Grid's built-in rowHoverColor parameter in aggrid-ix-theme-params.ts is set to transparent (or rgba(0, 0, 0, 0)) to prevent AG Grid from rendering its own hover overlay simultaneously, which would result in double-hover tinting.
.ag-row.ag-row-hover .ag-cell {
box-shadow: inset 0 0 0 9999px var(--theme-color-ghost--hover) !important;
}There was a problem hiding this comment.
rowHoverColor is intentionally retained; it and the cell tint layer together to produce consistent hover across zebra rows. Removing it regresses odd/even consistency.
The box-shadow: inset approach produces per-cell border seams, so it's not suitable here.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/aggrid/src/aggrid-ix-theme-params.ts (2)
11-28: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd a changeset for this aggrid theming change. This is user-facing styling, so it needs a scoped
.changesetentry.🤖 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/aggrid/src/aggrid-ix-theme-params.ts` around lines 11 - 28, Add a scoped changeset entry for the aggrid package describing the user-facing theming/styling change shown in the aggrid theme parameters. Use the repository’s standard changeset format and an appropriate patch-level bump.Sources: Coding guidelines, Path instructions
21-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet
rowHoverColortotransparentThe hover tint is already applied inrow-state.style.css, so leavingrowHoverColoronvar(--theme-color-ghost--hover)double-tints hovered striped rows and can wash out the odd/even backgrounds.🤖 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/aggrid/src/aggrid-ix-theme-params.ts` at line 21, Update the rowHoverColor setting in the theme parameters to use transparent instead of the theme ghost hover variable, leaving hover tinting to row-state.style.css and preserving the striped row backgrounds.
🤖 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 `@packages/storybook-docs/src/stories/aggrid.stories.ts`:
- Around line 38-49: Update renderGrid to retain the GridApi returned by
createGrid and destroy the previously stored grid instance before creating a
replacement. Ensure cleanup occurs on each re-render while preserving the
existing container setup and grid options.
---
Outside diff comments:
In `@packages/aggrid/src/aggrid-ix-theme-params.ts`:
- Around line 11-28: Add a scoped changeset entry for the aggrid package
describing the user-facing theming/styling change shown in the aggrid theme
parameters. Use the repository’s standard changeset format and an appropriate
patch-level bump.
- Line 21: Update the rowHoverColor setting in the theme parameters to use
transparent instead of the theme ghost hover variable, leaving hover tinting to
row-state.style.css and preserving the striped row backgrounds.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b235939-a7ec-4e75-9509-970ce14d1153
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/aggrid/src/aggrid-ix-theme-params.tspackages/aggrid/src/index.tspackages/aggrid/src/row-state.style.csspackages/storybook-docs/package.jsonpackages/storybook-docs/src/stories/aggrid.stories.ts
| function renderGrid(args: AgGridArgs) { | ||
| const container = document.createElement('div'); | ||
| container.style.height = '20rem'; | ||
| container.style.width = '100%'; | ||
|
|
||
| createGrid(container, { | ||
| ...args.gridOptions, | ||
| theme: getIxTheme(agGridCommunity), | ||
| }); | ||
|
|
||
| return container; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Store grid API and call destroy() on re-render to prevent leaks.
createGrid returns a GridApi with a destroy() method. Without storing and destroying the previous instance, Storybook re-renders (arg changes, story switching) will leak grid instances — event listeners, timers, and DOM observers remain active. This degrades Storybook performance over time.
♻️ Proposed fix to add grid cleanup
+let gridApi: ReturnType<typeof createGrid> | null = null;
+
function renderGrid(args: AgGridArgs) {
+ if (gridApi) {
+ gridApi.destroy();
+ gridApi = null;
+ }
const container = document.createElement('div');
container.style.height = '20rem';
container.style.width = '100%';
- createGrid(container, {
+ gridApi = createGrid(container, {
...args.gridOptions,
theme: getIxTheme(agGridCommunity),
});
return container;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function renderGrid(args: AgGridArgs) { | |
| const container = document.createElement('div'); | |
| container.style.height = '20rem'; | |
| container.style.width = '100%'; | |
| createGrid(container, { | |
| ...args.gridOptions, | |
| theme: getIxTheme(agGridCommunity), | |
| }); | |
| return container; | |
| } | |
| let gridApi: ReturnType<typeof createGrid> | null = null; | |
| function renderGrid(args: AgGridArgs) { | |
| if (gridApi) { | |
| gridApi.destroy(); | |
| gridApi = null; | |
| } | |
| const container = document.createElement('div'); | |
| container.style.height = '20rem'; | |
| container.style.width = '100%'; | |
| gridApi = createGrid(container, { | |
| ...args.gridOptions, | |
| theme: getIxTheme(agGridCommunity), | |
| }); | |
| return container; | |
| } |
🤖 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/storybook-docs/src/stories/aggrid.stories.ts` around lines 38 - 49,
Update renderGrid to retain the GridApi returned by createGrid and destroy the
previously stored grid instance before creating a replacement. Ensure cleanup
occurs on each re-render while preserving the existing container setup and grid
options.
|
Fixed via #2713 |
3 participants
💡 What is the current behavior?
GitHub Issue Number: #
Jira Issue Number: 4248
🆕 What is the new behavior?
🏁 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
Summary by CodeRabbit
New Features
Style