fix: always maintain proportional column sizes#628
Conversation
Currently, when first resizing a column and then resizing the entire table, the column sizes are reset to their initial value provided as an input. With this change, they keep their proportional size. Closes #617
Summary of ChangesHello @spike-rabbit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where manually resized columns would lose their adjusted widths and revert to initial sizes when the entire table was subsequently resized. The changes introduce a more robust column width management system that preserves the proportional distribution of column sizes, enhancing the user experience by making column resizing more predictable and persistent. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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
The pull request addresses a bug where column sizes were reset after resizing a column and then the entire table. The changes ensure that column sizes maintain their proportional values. The primary changes involve modifying the recalculateColumns method in datatable.component.ts to correctly handle column width distribution, especially when columnMode is set to 'force'. Additionally, the ngOnInit lifecycle hook was removed, and a new effect was added to trigger recalculateColumns when necessary, integrating better with Angular's signal-based reactivity model and ensuring type safety with WritableSignal types.
| readonly _internalColumns = linkedSignal(() => | ||
| this.recalculateColumns( | ||
| toInternalColumn( | ||
| this.columnTemplates().length | ||
| ? this.columnTemplates().map(c => c.column()) | ||
| : (this.columns() ?? []), | ||
| this._defaultColumnWidth | ||
| ) | ||
| toInternalColumn( | ||
| this.columnTemplates().length | ||
| ? this.columnTemplates().map(c => c.column()) | ||
| : (this.columns() ?? []), | ||
| this._defaultColumnWidth | ||
| ) |
There was a problem hiding this comment.
The recalculateColumns call is removed from the _internalColumns linked signal. This change is crucial as it prevents recalculateColumns from being called within a computed context, which could lead to unintended side effects or infinite loops if recalculateColumns itself triggers signal updates. The new effect in the constructor now handles the column recalculation.
| it('should maintain proportional column sizes on window resize after column resize', async () => { | ||
| component.columns.set([ | ||
| { prop: 'A', width: 100 }, | ||
| { prop: 'B', width: 100 }, | ||
| { prop: 'C', width: 100 }, | ||
| { prop: 'D', width: 100 } | ||
| ]); | ||
| component.columnMode.set('force'); | ||
| await fixture.whenStable(); | ||
| const headerCells = fixture.debugElement.queryAll(By.directive(DataTableHeaderCellComponent)); | ||
| const cellSizes = () => headerCells.map(cell => cell.nativeElement.clientWidth); | ||
| headerCells[1].triggerEventHandler('resize', { | ||
| width: 150, | ||
| column: headerCells[1].componentInstance.column() | ||
| }); | ||
|
|
||
| await fixture.whenStable(); | ||
| expect(cellSizes()).toEqual([100, 150, 75, 75]); | ||
| component.size.set(300); | ||
| await fixture.whenStable(); | ||
| fixture.debugElement.query(By.directive(DatatableComponent)).componentInstance.recalculate(); | ||
| await fixture.whenStable(); | ||
| expect(cellSizes()).toEqual([75, 125, 50, 50]); | ||
| }); |
There was a problem hiding this comment.
This new test case effectively covers the scenario described in the PR description. It correctly simulates column resizing and then table resizing to ensure proportional column sizes are maintained. The use of cellSizes() to map and compare client widths is a good approach for verifying the visual state.
| numberAttribute, | ||
| OnDestroy, | ||
| OnInit, | ||
| output, |
There was a problem hiding this comment.
The OnInit import is removed here. This aligns with the removal of the ngOnInit lifecycle hook in the DatatableComponent class.
| export class DatatableComponent<TRow extends Row = any> | ||
| implements OnInit, DoCheck, AfterViewInit, OnDestroy | ||
| implements DoCheck, AfterViewInit, OnDestroy | ||
| { |
There was a problem hiding this comment.
The OnInit interface is removed from the DatatableComponent's implemented interfaces. This is consistent with the removal of the ngOnInit method, indicating that initial setup logic has been refactored elsewhere, likely into effects or the constructor.
| // if the table is hidden the visibility | ||
| // listener will invoke this itself upon show | ||
| this.recalculate(); | ||
| effect(() => this.recalculateColumns()); |
There was a problem hiding this comment.
Adding an effect to call recalculateColumns() is a good way to ensure that column widths are re-evaluated when dependencies change. This replaces the previous ngOnInit call and integrates better with Angular's signal-based reactivity model.
| this.recalculateDims(); | ||
| this._internalColumns.set(this.recalculateColumns(this._internalColumns().slice())); | ||
| this.cd.markForCheck(); | ||
| } |
There was a problem hiding this comment.
The recalculate method is simplified to only call recalculateDims(). The removal of _internalColumns.set and cd.markForCheck() from here is appropriate, as column recalculation is now handled by the new effect and change detection is managed by Angular's signal system.
1 participant
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
Currently, when first resizing a column and then resizing the entire table, the column sizes are reset to their initial value provided as an input.
What is the new behavior?
With this change, they keep their proportional size.
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information:
Closes #617