ghiscoding / Angular-Slickgrid

Angular-Slickgrid is a wrapper of the lightning fast & customizable SlickGrid datagrid, it also includes multiple Styling Themes
https://ghiscoding.github.io/Angular-Slickgrid
Other
397 stars 120 forks source link

Detail row content is cleared when the grid columns are changed #1476

Closed joostvanbijsterveld closed 1 month ago

joostvanbijsterveld commented 1 month ago

Describe the bug

In a grid with detail row functionality it is possible to expand the detail row. The content in the detail area of the row is shown based on the data of the row. However the content is no longer displayed when the columns of the grid are changed.

Reproduction

Bezigmetopnemen2024-09-25143619-ezgif com-video-to-gif-converter

Expectation

The content of the detail row should remain visible when the columns of the grid are changed.

Environment Info

Angular 18.2.2
Angular-Slickgrid 8.6.0
Typescript 5.4.2
Google Chrome 129.0.6668.60
Windows 11 10.0.22631

Validations

ghiscoding commented 1 month ago

Yeah Row Detail is a very special thing, every time the grid UI changes it has to call the redrawAllViewComponents() method and it looks like some command of the Grid Menu calls the redraw (like column reordered) but it's not being called for the column selection, it should be added for the onColumnsChanged event as well (I'm assuming you'll have the same problem with the Column Picker as well). For now you can call redrawAllViewComponents() yourself, see Row Detail - Calling Addon Methods Dynamically

this.gridOptions = {
  columnPicker: {
    onColumnsChanged: (_e, args) => rowDetailInstance.redrawAllViewComponents()
  },
  gridMenu: {
    onColumnsChanged: (_e, args) => rowDetailInstance.redrawAllViewComponents()
  },
}

https://github.com/ghiscoding/Angular-Slickgrid/blob/f9a96bf1caa95b3173dec1c46bc0583f4685a55c/src/app/modules/angular-slickgrid/extensions/slickRowDetailView.ts#L205-L212

A PR fix would be welcome, I'm assuming something like this should fix the issue (unit tests would also need to be updated)

this._subscriptions.push(
  this.eventPubSubService?.subscribe('onFilterChanged', this.redrawAllViewComponents.bind(this)),
+ this.eventPubSubService?.subscribe('onGridMenuColumnsChanged', this.redrawAllViewComponents.bind(this)),
+ this.eventPubSubService?.subscribe('onColumnPickerColumnsChanged', this.redrawAllViewComponents.bind(this)),
  this.eventPubSubService?.subscribe('onGridMenuClearAllFilters', () => window.setTimeout(() => this.redrawAllViewComponents())),
  this.eventPubSubService?.subscribe('onGridMenuClearAllSorting', () => window.setTimeout(() => this.redrawAllViewComponents())),
);

This Row Detail really has to be redrawn every time the UI changes and it happens so, so many times, I'm sure that there's even more events that are missed.

joostvanbijsterveld commented 1 month ago

Thank you for the solution, I hadn't gotten around to creating a PR myself yet.

ghiscoding commented 1 month ago

no problem, I actually wanted to test with the grid.onRendered event, it is being called more often covering most use cases. And if you haven't already, you can ⭐ if you like the project, cheers 😉

ghiscoding commented 1 month ago

So it turns out that using the grid.onRendered event is not ideal because this event is actually called way too often (it's called even when scrolling up/down) and this makes it worst on the perf side because we should only re-render the Row Detail when it comes back into the view (i.e. scroll to the bottom of the grid and then scrolling back to top, we should re-render at that time only, however with the scroll it was re-rendering a lot of times and this is bad).... So I will revert the PR #1480 and instead use the other 2 events that I proposed earlier in another PR.