Skip to content

Conversation

@KoolADE85
Copy link
Contributor

This PR upgrades AG Grid to version 33.

@KoolADE85 KoolADE85 changed the base branch from main to refactor/functional-component July 15, 2025 16:00
@AnnMarieW
Copy link
Collaborator

AnnMarieW commented Jul 15, 2025

Hi @KoolADE85

I think there was a discussion about AG Grid supporting V32 for the long term because there are so many breaking changes going from v32 to v33. However, going from v33 to v34 does not have any breaking changes, so it might actually be better to do v34 and skip v33. https://www.ag-grid.com/react-data-grid/upgrading-to-ag-grid-34/

This won't have a big impact on our docs because most of the new features are for Enterprise and they aren't in the dash-docs.

@KoolADE85
Copy link
Contributor Author

Hi @KoolADE85

I think there was a discussion about AG Grid supporting V32 for the long term because there are so many breaking changes going from v32 to v33. However, going from v33 to v34 does not have any breaking changes, so it might actually be better to do v34 and skip v33. https://www.ag-grid.com/react-data-grid/upgrading-to-ag-grid-34/

This won't have a big impact on our docs because most of the new features are for Enterprise and they aren't in the dash-docs.

Good call! I have some test failures around filters when I use v34. I'm going to investigate whether it's the test or the component itself that needs updating.

@KoolADE85 KoolADE85 marked this pull request as ready for review July 15, 2025 17:01
@gvwilson gvwilson requested a review from T4rk1n July 18, 2025 13:07
@gvwilson gvwilson added feature something new P1 needed for current cycle labels Jul 18, 2025
Base automatically changed from refactor/functional-component to main July 18, 2025 16:30
@BSd3v
Copy link
Collaborator

BSd3v commented Jul 18, 2025

Before we do this, I need we need to branch v32 from main.

@BSd3v
Copy link
Collaborator

BSd3v commented Jul 18, 2025

Also, we should look at this, if this PR is good, should probably do theirs. :)

#355

@KoolADE85 KoolADE85 force-pushed the feature/ag-grid-33 branch from 0d79034 to 8c67d20 Compare July 18, 2025 22:37
@BSd3v
Copy link
Collaborator

BSd3v commented Jul 18, 2025

Should make sure there is a way to set the theme to legacy though. So that people can opt for not having to create the theme.

@KoolADE85
Copy link
Contributor Author

Should make sure there is a way to set the theme to legacy though. So that people can opt for not having to create the theme.

OK, this gets a little tricky since AG Grid doesn't support importing legacy theme CSS files (as we were previously) AND setting a modern theme object.

So in my update, I propose that anyone who wants to keep using a legacy CSS theme, to import the CSS by hand (we won't automatically import, for example, ag-grid.css. An example of this is in tests/examples/themes_legacy.py.

Our codebase going forward, would support AG Grid's modern theme system by default. See tests/examples/themes.py.

This approach seems right because, in case of any errors/warnings, the message that AG Grid outputs in the console matches what users would need to do to their dash app in order to get the grid looking right.

@BSd3v
Copy link
Collaborator

BSd3v commented Jul 21, 2025

Should make sure there is a way to set the theme to legacy though. So that people can opt for not having to create the theme.

OK, this gets a little tricky since AG Grid doesn't support importing legacy theme CSS files (as we were previously) AND setting a modern theme object.

So in my update, I propose that anyone who wants to keep using a legacy CSS theme, to import the CSS by hand (we won't automatically import, for example, ag-grid.css. An example of this is in tests/examples/themes_legacy.py.

Our codebase going forward, would support AG Grid's modern theme system by default. See tests/examples/themes.py.

This approach seems right because, in case of any errors/warnings, the message that AG Grid outputs in the console matches what users would need to do to their dash app in order to get the grid looking right.

We can still support these files and not import them directly into the grid the same way. There are a couple of repos that do this in order to make it easier to import these files that are otherwise dependencies to make the package load properly.

@KoolADE85
Copy link
Contributor Author

Alright, I'm giving this a try: lazy load the css for legacy themes if the user has specified a className (and no theme). Otherwise, default to AG Grid's modern theme system.
There's quite a bit of extra code to keep this backwards-compatible API, but it does achieve the goal of allowing users to upgrade without needing to refactor their theming.
What do you think?

@KoolADE85 KoolADE85 force-pushed the feature/ag-grid-33 branch 2 times, most recently from 60d5741 to 744b9e5 Compare July 22, 2025 17:21
@KoolADE85 KoolADE85 force-pushed the feature/ag-grid-33 branch from 744b9e5 to 739b1b7 Compare July 22, 2025 17:24
@BSd3v BSd3v linked an issue Jul 22, 2025 that may be closed by this pull request
@BSd3v
Copy link
Collaborator

BSd3v commented Jul 23, 2025

Also, we should look at this, if this PR is good, should probably do theirs. :)

#355

It seems like #355 is covered by this current PR, with a couple of other features.

@KoolADE85
Copy link
Contributor Author

Also, we should look at this, if this PR is good, should probably do theirs. :)
#355

It seems like #355 is covered by this current PR, with a couple of other features.

Yes, I think that one can be closed in favour of this one - we have theme support now here, plus latest patch release of 33.

Copy link
Collaborator

@BSd3v BSd3v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💃

@KoolADE85 KoolADE85 merged commit 6622d5f into main Jul 23, 2025
3 checks passed
@KoolADE85 KoolADE85 deleted the feature/ag-grid-33 branch July 23, 2025 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature something new P1 needed for current cycle

5 participants