feat(ix-aggrid): Update ix-aggrid to use aggrid v33 #2198
Conversation
🦋 Changeset detectedLatest commit: 8923f74 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
/gemini review |
![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 does a great job of updating ix-aggrid to support AG Grid v33 and its new theming API. The transition from a Gulp-based build process to Rollup is a significant improvement, and the new theming implementation using useIxTheme is well-structured. The changes are extensive, touching build configurations, dependencies, styling, and tests.
I've found a few areas for improvement. Most critically, the test file packages/aggrid/tests/basic.html appears to be outdated. It seems to be using the old theming approach and an incorrect version of AG Grid, which could lead to tests not covering the new implementation. I strongly recommend updating this file to align with the rest of the changes in this PR.
Additionally, I've added a few suggestions with medium severity to improve code readability and maintainability.
Overall, this is a solid update that modernizes the package. Addressing the feedback will help ensure the changes are robust and maintainable.
…into feat/aggrid-v33-theme
Co-authored-by: Lukas Maurer <[email protected]>
danielleroux
left a comment
danielleroux
left a comment
There was a problem hiding this comment.
@lzeiml Looks like some breaking changes documentation is missing. Like removal of the themeSwitcher etc. Maybe you can take over parts from the migration guide but in the v4 breaking changes markdown should also be contain some information about it.
lzeiml
force-pushed
the
feat/aggrid-v33-theme
branch
from
f8b1481 to
7d9b231
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and well-executed update to ix-aggrid, migrating it to support AG Grid v33+ and its new theming API. This is a major improvement, moving from CSS overrides to a more maintainable, parameter-based approach. The changes are comprehensive, including a build system refactor to Rollup, dependency updates, and a complete rewrite of the theme implementation. The example applications for Angular, React, and Vue have also been correctly updated to reflect the new API usage. My review identified a couple of minor redundancies in the theme parameter definitions that should be addressed for cleanliness and to prevent potential issues.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
💡 What is the current behavior?
Currently the new aggrid theming API (v32+) is not supported.
GitHub Issue Number: #1784
🆕 What is the new behavior?
Use new aggrid theming API
🏁 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)