fix(datatable-body): avoid multiple calculations and update properly on changes #107
Conversation
Killusions
changed the title
Killusions
force-pushed
the
fix/avoid-multiple-calculations-and-update-properly-on-changes-in-datatable-body
branch
from
2c99d95
to
aab60d4
Compare
Killusions
force-pushed
the
fix/avoid-multiple-calculations-and-update-properly-on-changes-in-datatable-body
branch
from
aab60d4
to
3f4280e
Compare
|
@chintankavathia It seems due to the |
Just made it wait for it |
|
@chintankavathia Before I go ahead here, can you check if it conflicts with something? |
There was a problem hiding this comment.
@Killusions some of things here would conflict with some of the refactoring which I am currently doing. I was wondering instead of replacing setters with ngOnChanges we wait for angular v19 which is just few weeks away and then switch to signal inputs, what do you think? We anyway agreed to bump to v19 before we do the major release.
| <div | ||
| class="datatable-row-{{ colGroup.type }} datatable-row-group" | ||
| [ngStyle]="_groupStyles[colGroup.type]" | ||
| [ngStyle]="groupStyles[colGroup.type]" |
There was a problem hiding this comment.
this will conflict with changes I have here https://github.com/siemens/ngx-datatable/pull/106/files#diff-4e9a953025090dc47141b72d55a278ab32d1f3e1d3505c04fc15ca6228240a6eR36
| private updateColumnsByPin() { | ||
| this._columnsByPin = columnsByPinArr(this.columns); | ||
| } | ||
|
|
||
| private updateGroupStyles() { | ||
| this._groupStyles.left = this.calcStylesByGroup('left'); | ||
| this._groupStyles.center = this.calcStylesByGroup('center'); | ||
| this._groupStyles.right = this.calcStylesByGroup('right'); | ||
| this.cd.markForCheck(); | ||
| } | ||
|
|
||
| private calcStylesByGroup(group: 'left' | 'right' | 'center') { | ||
| const widths = this.columnGroupWidths; | ||
| const offsetX = this.offsetX; | ||
|
|
||
| if (group === 'left') { | ||
| return { | ||
| width: `${widths[group]}px`, | ||
| ...translateXY(offsetX, 0) | ||
| }; | ||
| } else if (group === 'right') { | ||
| const bodyWidth = this.innerWidth; | ||
| const totalDiff = widths.total - bodyWidth; | ||
| const offsetDiff = totalDiff - offsetX; | ||
| const offset = | ||
| (offsetDiff + (this.verticalScrollVisible ? this.scrollbarHelper.getWidth() : 0)) * -1; | ||
| return { | ||
| width: `${widths[group]}px`, | ||
| ...translateXY(offset, 0) | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| width: `${widths[group]}px` | ||
| }; | ||
| } |
There was a problem hiding this comment.
this will not be needed with #106
There was a problem hiding this comment.
@chintankavathia Makes sense, but the main part of this still stands, which is to avoid doing calculations in every row and doing them shared in the body component instead.
This starts to improve the issues caused by the many setters and getters, while it does not fully solve some of the OnPush issues I encountered, it at least helps with some them.