Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AG Grid popup heights not recalculated on density change #2479

Closed
2 of 9 tasks
Tracked by #2104
libertymayc opened this issue Sep 19, 2023 · 1 comment
Closed
2 of 9 tasks
Tracked by #2104

AG Grid popup heights not recalculated on density change #2479

libertymayc opened this issue Sep 19, 2023 · 1 comment

Comments

@libertymayc
Copy link
Contributor

libertymayc commented Sep 19, 2023

Package name(s)

AG Grid Theme (@salt-ds/ag-grid-theme)

Package version(s)

v1.3.0

Description

According to https://www.ag-grid.com/javascript-data-grid/global-style-customisation-compactness/?#changing-row-and-header-heights-at-runtime, changing --ag-list-item-height should force cached calculated values to recalculate. The demo on this page works for this.

But, from what I have been experiencing, the values aren't being recalculated for us with this method... which causes a significant issue when the density is changed (e.g., density changed HD -> TD - see overlap on sunshine icon):

Screenshot 2023-09-19 at 3 12 17 PM

I'm not sure if this could be because we need to upgrade ag grid? I can't find this being linked to a version. #2469 shows that the solution described in the ag-grid docs above not working in Storybook. I've also tried adding extra classes just for this value change (--ag-list-item-height) and changing it on ag-popup itself, to no avail. Notice how the height is being calculated so we can't apply our own styles (which makes sense, we should just be able to change the --ag-list-item-height to cause it to be recalculated as explained above)

Screenshot 2023-09-19 at 4 03 06 PM

The row and header height are fine changing between densities because of the API callbacks provided https://github.com/jpmorganchase/salt-ds/blob/main/packages/ag-grid-theme/stories/dependencies/useAgGridHelpers.ts#L65. It seems Rustam probably already knew of this being an issue and also couldn't find a fix: https://github.com/jpmorganchase/salt-ds/blob/main/packages/ag-grid-theme/stories/dependencies/useAgGridHelpers.ts#L68. Weirdly, ag grid seems to only allow you to set header and row height here, despite listing --ag-list-item-height as a variable within the page https://www.ag-grid.com/javascript-data-grid/global-style-customisation-compactness. And api.getSizesForCurrentTheme() also only returns row and header height.

Note: The values are only calculated when the pop up shows up, so any filters that haven't already been opened won't be affected by density change.
Related:
https://stackoverflow.com/questions/73877337/increase-the-height-of-filter-list-ag-grid-set-filter
https://stackoverflow.com/questions/56293765/aggrid-how-to-set-the-line-height-of-a-ag-virtual-list-item-inside-a-sidebar-fi

This might be something that can be fixed in #2103

Operating system

  • macOS
  • Windows
  • Linux
  • iOS
  • Android

Browser

  • Chrome
  • Safari
  • Firefox
  • Edge
@libertymayc libertymayc added type: bug 🪲 Something isn't working status: awaiting triage Automatically added to new issues. Should be removed once they have been triaged. package: ag-grid-theme labels Sep 19, 2023
@origami-z origami-z removed the status: awaiting triage Automatically added to new issues. Should be removed once they have been triaged. label Oct 4, 2023
@origami-z
Copy link
Contributor

Resolved by #3195

@joshwooding joshwooding removed the type: bug 🪲 Something isn't working label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants