feat(platform): table optimisation #9991

N1XUS · 2023-06-07T11:48:55Z

Related Issue(s)

closes #9932
closes #9926
closes #9955
closes #9746

Description

Main idea behind this refactoring was:

Reduce amount of recompiled code during development (Hot Module Reloading). This is achieved by ejecting helper classes, directives, models from table package into a separate table-helpers.
Reduce amount of html code inside table template. this is achieved by ejecting table row related code into a separate components;
Reduce amount of business logic inside table component itself. This is achieved by ejecting logical parts of code into a separate directives that. For example, virtual scroll directive would be initiated only if table element has virtualScroll property. Otherwise it will not be loaded at all, same goes to dnd support. Initial state and dataSource are always applied since it's a core functionality of the table;
Reduce amount of change detection cycles:
- Reduce triggers during event emission: now event listeners run outside zone, and triggering the change only when previous calculation result is different than current (for example, resize functionality: previously checked table and it's related components on every mousemove, now it reacts only if resize is started or finished. It will not trigger detection during the resize process).
- Reduce triggers during table property changes: previously we ran change detection for whole table to recalculate everything. Now we can call it per each row if needed. Each row reacts on it's own data, so we shouldn't have 100+ detections at once when something changes for a particular row or cell inside it.
- Rely more on async objects, such as visible rows array, table loading state, row checked state, etc.
- Reduce method calls. Previously we had function evaluation for some properties (for example, getting element ref, checking whether the cell is the first in the row, etc.). That approach added tiny delays in function evaluation, thus, slowing down the detection itself. Now mostly we rely on calculated properties, which are being calculated only when the related properties are changed.
- Reduce component re-initialization: Platform table now tries to reuse existing table row component instance and pass new data to it rather than re-create new component instance with it's view (thus reducing createEmbedView calls for row, inner cells, etc.);

Platform table refactoring includes following breaking changes:

Components that rely on HasElementRef interface:

HasElementRef interface has been changed. Now instead of elementRef() method, components should have elementRef: ElementRef property. This can include getters too;

Core table:

TableCellDirective now directly extends from FocusableItemDirective. Previously it had FocusableItemDirective as a host directive applied to itself;

Platform table:

All directives, services, models, data source classes are moved from @fundamental-ngx/platform/table into a separate package @fundamental-ngx/platform/table-helpers. To keep backwards compatibility, @fundamental-ngx/platform/table re-exports @fundamental-ngx/platform/table-helpers so that the import paths are the same as before;
Platform table directives are standalone now;
Platform table data-source related functionality has been moved to TableDataSourceDirective standalone directive. Input properties are preserved;
Platform table drag&drop related functionality has been moved to TableDraggableDirective standalone directive. Input properties are preserved;
Platform table initial state related functionality has been moved to TableInitialStateDirective standalone directive. Input properties are preserved;
Platform table virtual scroll related functionality has been moved to TableVirtualScrollDirective standalone directive. Input properties are preserved;
Platform table data source has been refactored to utilize @fundamental-ngx/cdk/data-source implementation. Backwards compatibility is preserved for classes that extend from data-source/data-provider. More complex classes may need to be refactored according to new class signature;
Platform table now uses TableRowImpl class as implementation of TableRow interface instead of simple object;
Helper methods from TableComponent has been moved to a standalone functions;
SearchInput type is now not re-exported from @fundamental-ngx/platform/table. Instead use @fundamental-ngx/platform/search-field;
Table component html is now split between logical parts: table, table header rows, table content rows. Resulting markup is not changed, but the business logic parts are being moved to appropriate components;
Platform table rows now try to reuse the html element they bounded to. This means that if developers use reference to elementRef of the row, and rows array changes, they need to manually recalculate the reference to the element;
Platform table group row: text pattern changed from {{ column.name }} : {{ column.value }} to {{ column.name }}: {{ column.value }}. Space before colon has been removed;
Platform table input properties recalculation now more efficient. Previously we checked property value on every mousemove, now, only when something inside the table itself is changed. This may require additional detectChanges calls from developers who are using Platform Table.
Such input properties has been removed from platform table class:
- dataSource - setter available via fdp-table[dataSource] but class property now accessible from _dataSourceDirective property of Table component;
- state, initialVisibleColumns, initialSortBy, initialFilterBy, initialGroupBy, initialPage setters available via fdp-table[property] but the class property now accessible from initialState property of Table component;
- isTreeTable, enableRowReordering, dropMode setters available via fdp-table[property] but the class property now accessible via _dndTableDirective property of Table component;
- virtualScroll, renderAhead setter available via fdp-table[virtualScroll], but the class property now accessible via _virtualScrollDirective property of Table component;
Such events has been removed from Platform Table class:
- rowsRearrange - listener still available via fdp-table(rowsRearrange), but actual eventEmitter is now located in _dndTableDirective property of Table component;
- onDataRequested, onDataReceived - listeners still available via fdp-table(event), but actual eventEmitters are now located in _dataSourceDirective property of Table component;
Private API changed. For more information refer to this PR

Screenshots

Before:

After:

Please check whether the PR fulfills the following requirements

During Implementation

Visual Testing:

visual misalignments/updates
check Light/Dark/HCB/HCW themes
RTL/LTR - proper rendering and labeling
responsiveness(resize)
Content Density (Cozy/Compact/(Condensed))
States - hover/disabled/focused/active/on click/selected/selected hover/press state
Interaction/Animation - open/close, expand/collapse, add/remove, check/uncheck
Mouse vs. Keyboard support
Text Truncation

API and functional correctness

check for console logs (warnings, errors)
API boundary values
different combinations of components - free style
change the API values during testing

Documentation and Example validations

missing API documentation or it is not understandable
poor examples
Stackblitz works for all examples

Accessibility testing
Browser Testing - Edge, Safari, Chrome, Firefox

PR Quality

the commit message(s) follows the guideline:
https://github.com/SAP/fundamental-ngx/blob/main/CONTRIBUTING.md
tests for the changes that have been done
all items on the PR Review Checklist are addressed :
https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist
Run npm run build-pack-library and test in external application
update README.md
Breaking Changes Wiki

netlify · 2023-06-07T11:53:18Z

✅ Deploy Preview for fundamental-ngx ready!

Name	Link
🔨 Latest commit	`63ae765`
🔍 Latest deploy log	https://app.netlify.com/sites/fundamental-ngx/deploys/648999e231a1cf0008fd4f46
😎 Deploy Preview	https://deploy-preview-9991--fundamental-ngx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

…at/table-optimisation # Conflicts: # libs/cdk/src/lib/utils/drag-and-drop/dnd-list/dnd-list.directive.ts # libs/core/src/lib/feed-list-item/components/item/feed-list-item.component.spec.ts # libs/core/src/lib/message-box/message-box.component.spec.ts # libs/docs/cdk/drag-n-drop/dnd-docs.component.html # libs/platform/src/lib/table/table.component.ts # libs/platform/src/lib/table/table.module.ts

github-actions · 2023-06-07T15:10:55Z

Visit the preview URL for this PR (updated for commit 63ae765):

https://fundamental-ngx-gh--pr9991-feat-table-optimisat-aouudzk4.web.app

_{(expires Sat, 17 Jun 2023 10:55:33 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 41b993ee8e451bd7c6770b342ce142dc886eacff}

InnaAtanasova · 2023-06-12T20:10:06Z

The problems I mentioned are not a regression, they are also present in main. I created an issue for these cases: #10021

mikerodonnell89

Loading state boolean displaying as "true" even when the table is not loading:

mikerodonnell89 · 2023-06-13T16:27:05Z

Initial state example - can no longer focus the true/false group by rows via keyboard

Screen.Recording.2023-06-13.at.10.26.05.AM.mov

libs/cdk/src/lib/utils/directives/selectable-list/selection.service.spec.ts

g-cheishvili · 2023-06-13T18:20:44Z

libs/platform/src/lib/table-helpers/directives/table-cell-resizable.directive.ts _cdr is not used

libs/platform/src/lib/table/components/table-p13-dialog/filtering/filter-rule.component.html *ngSwitchCaseDefault -> *ngSwitchDefault

fundamental-ngx/libs/platform/src/lib/table/components/table-row/table-row.component.ts

Line 119 in 0d898c8

this._focusbleCellItems = items;

- not used anywhere

fundamental-ngx/libs/platform/src/lib/table/components/table-group-row/table-group-row.component.html

Line 45 in 043d173

    
           <ng-container *ngIf="column?.headerCellTemplate; else defaultHeaderCellTemplate">

- Here we have a type mismatch thing happening, I would say that we should have a small helper function or used $any to fight these warnings

fundamental-ngx/libs/platform/src/lib/table/components/table-header-row/table-header-row.component.ts

Lines 153 to 158 in 0d898c8

    
           /** @hidden */ 
        
           _toggleAllSelectableRows(event: Event): void { 
        
               event.stopImmediatePropagation(); 
        
               this._tableRowService.toggleAllSelectableRows(!this.checkedState); 
        
           }

- we do not use this and seems like it is a private function, so why not remove this?

fundamental-ngx/libs/platform/src/lib/table/table.component.html

Line 73 in 180d05b

    
           *ngFor="let rowIndex of _tableRowsInViewPortPlaceholder; let rowIdx = index; trackBy: _rowTrackBy"

- unused rowIdx

fundamental-ngx/libs/platform/src/lib/table/table.component.ts

Line 561 in 0ae7728

_tableRowsInViewport: TableRow<T>[] = [];

- unused property

fundamental-ngx/libs/platform/src/lib/table-helpers/directives/table-cell-resizable.directive.ts

Lines 90 to 94 in 92e1f43

    
           const data = this._getResizer(event) || { resizerPosition: 0, resizedColumn: this.columnName }; 
        
           if (equal(data, this._prevData)) { 
        
               return; 
        
           }

- this can be done in distinctUntilChanged pipe callback function. First, map to the desired structure, then just distinctUntilChanged. This way you can even avoid saving prevData

Could not find any circular dependencies, which is awesome, considering the functionality over here, nice work

g-cheishvili · 2023-06-13T19:08:20Z

fundamental-ngx/libs/platform/src/lib/table-helpers/directives/table-scrollable.directive.ts

Line 26 in 1082296

private _destroyed = new Subject<void>();

- Why not use our service, at least until we upgrade to the 16th version, then we will do DestroyRef instead

fundamental-ngx/libs/platform/src/lib/table-helpers/directives/table-scrollable.directive.ts

Line 138 in 1082296

const activeEl = document.activeElement;

this can be replaced with this.renderer.selectRootElement(this.elementRef.nativeElement).ownerDocument.activeElement. Renderer is Renderer2 instance.

fundamental-ngx/libs/platform/src/lib/table-helpers/directives/table-scrollable.directive.ts

Lines 130 to 180 in 1082296

    
           scrollToOverlappedCell(rtl: boolean, freezableColumnsSize: number, freezableEndColumnSize: number): void { 
        
               const tableScrollableEl = this.elementRef.nativeElement; 
        
               const isRtl = rtl; 
        
               if ( 
        
                   (freezableColumnsSize || freezableEndColumnSize) && 
        
                   tableScrollableEl.scrollWidth > tableScrollableEl.clientWidth 
        
               ) { 
        
                   const activeEl = document.activeElement; 
        
                   if ( 
        
                       activeEl && 
        
                       !( 
        
                           activeEl.classList.contains('fd-table__cell--fixed') || 
        
                           activeEl.classList.contains('fd-table__cell--fixed-end') 
        
                       ) 
        
                   ) { 
        
                       if (freezableColumnsSize && !freezableEndColumnSize) { 
        
                           activeEl.scrollIntoView({ block: 'nearest', inline: 'end' }); 
        
                       } else if (!freezableColumnsSize && freezableEndColumnSize) { 
        
                           activeEl.scrollIntoView({ block: 'nearest', inline: 'center' }); 
        
                       } else if (freezableColumnsSize && freezableEndColumnSize) { 
        
                           // check to see if another element obstructs the active element 
        
                           const activeElLeft = activeEl.getBoundingClientRect().left; 
        
                           const activeElTop = activeEl.getBoundingClientRect().top; 
        
                           const topElementFromLeft = document.elementFromPoint(activeElLeft, activeElTop); 
        
                           // if the activeEl is overlapped 
        
                           if ( 
        
                               topElementFromLeft && 
        
                               !activeEl.isSameNode(topElementFromLeft) && 
        
                               topElementFromLeft.classList.contains('fd-table__cell--fixed-end') 
        
                           ) { 
        
                               const topElementX = topElementFromLeft.getBoundingClientRect().left; 
        
                               const leftVal = isRtl 
        
                                   ? (activeElLeft + activeEl.getBoundingClientRect().width - topElementX) * -1 
        
                                   : activeElLeft + activeEl.getBoundingClientRect().width - topElementX; 
        
                               tableScrollableEl.scrollBy({ top: 0, left: leftVal }); 
        
                           } else if ( 
        
                               topElementFromLeft && 
        
                               !activeEl.isSameNode(topElementFromLeft) && 
        
                               topElementFromLeft.classList.contains('fd-table__cell--fixed') 
        
                           ) { 
        
                               const topElementX = topElementFromLeft.getBoundingClientRect().right; 
        
                               const leftVal = isRtl 
        
                                   ? (activeElLeft - activeEl.getBoundingClientRect().width - topElementX) * -1 
        
                                   : activeElLeft - activeEl.getBoundingClientRect().width - topElementX; 
        
                               tableScrollableEl.scrollBy({ top: 0, left: leftVal }); 
        
                           } 
        
                       } 
        
                   } 
        
               } 
        
           }

This is some very hard to digest thing here. I’d love to see it simplified, but I do not know what or how exactly :/ Maybe divided into smaller functions or something?

fundamental-ngx/libs/platform/src/lib/table/table.component.ts

Lines 670 to 673 in 0ae7728

    
           this.initialState?.setTable(this); 
        
           this._dndTableDirective?.setTable(this); 
        
           this._virtualScrollDirective?.setTable(this); 
        
           this._dataSourceDirective?.setTable(this);

All of these are fdp-table[something] family of the directives, so they could have done _table = inject(Table, {host: true}) and skip this setting step from table constructor. Why are you doing this this way? Is there any reason I do not see ?

N1XUS · 2023-06-13T21:06:45Z

@g-cheishvili on the last item of the comment - yes, it's can't be done differently since it will bring circular dependency (I've tried splitting, and this is the best aproach at the moment)

g-cheishvili · 2023-06-13T21:33:24Z

@g-cheishvili on the last item of the comment - yes, it's can't be done differently since it will bring circular dependency (I've tried splitting, and this is the best aproach at the moment)

Even if you use InjectionToken it still recognizes circularity?

N1XUS · 2023-06-14T06:31:55Z

@g-cheishvili on the last item of the comment - yes, it's can't be done differently since it will bring circular dependency (I've tried splitting, and this is the best aproach at the moment)

Even if you use InjectionToken it still recognizes circularity?

Yes.

N1XUS · 2023-06-14T06:52:30Z

Initial state example - can no longer focus the true/false group by rows via keyboard

Screen.Recording.2023-06-13.at.10.26.05.AM.mov

fixed

N1XUS · 2023-06-14T07:37:36Z

@g-cheishvili:
- Here we have a type mismatch thing happening, I would say that we should have a small helper function or used $any to fight these warnings -> I wouldn't say that calling helper function for each row simply to remove IDE warning is a performance improvement.

g-cheishvili

Besides this

fundamental-ngx/libs/platform/src/lib/table-helpers/directives/table-cell-resizable.directive.ts

Line 80 in 4263d33

distinctUntilChanged(),

I am more than happy to include this in the main. Amazing job

reported issues are fixed

N1XUS added 4 commits May 28, 2023 10:31

feat(cdk,platform): dnd predicate

d58aa23

fix(platform): fix dnd event

2fcad1a

feat(platform): optimise change detection

3309a3a

fix(cdk,platform): correct dnd item resolve

aa8c7dc

N1XUS requested a review from mikerodonnell89 as a code owner June 7, 2023 11:48

N1XUS self-assigned this Jun 7, 2023

N1XUS requested review from droshev and InnaAtanasova as code owners June 7, 2023 11:48

feat(platform): table optimization

3a316ce

N1XUS force-pushed the feat/table-optimisation branch from 9ed5dad to 3a316ce Compare June 7, 2023 14:10

N1XUS added 2 commits June 7, 2023 17:26

feat(platform): table optimization

1082296

droshev requested a review from fkolar June 7, 2023 14:49

droshev added the table label Jun 7, 2023

droshev added this to the Sprint 114 milestone Jun 7, 2023

N1XUS added 2 commits June 7, 2023 17:58

fix(platform): fix build

e6ac664

fix(platform): fix failing unit test

d7091d7

fix(platform): fix failing unit test

92e1f43

N1XUS changed the title ~~Feat/table optimisation~~ feat(platform): table optimisation Jun 8, 2023

N1XUS added 2 commits June 8, 2023 11:20

feat(platform): minor improvements

2ca6075

Merge remote-tracking branch 'origin/main' into feat/table-optimisation

4f23986

N1XUS force-pushed the feat/table-optimisation branch from 4558737 to 54cebae Compare June 8, 2023 11:31

feat(platform): minor improvements

819f070

N1XUS force-pushed the feat/table-optimisation branch from 54cebae to 819f070 Compare June 8, 2023 11:35

N1XUS added 3 commits June 9, 2023 10:24

fix(platform): fix not able to dnd after cancel

bdf8c72

fix(platform): simplify property checks

ef9fb92

fix(platform): fix build

cfcb961

fix(platform): (de)select all enhancements

90d9759

N1XUS requested a review from g-cheishvili June 13, 2023 09:26

N1XUS added 2 commits June 13, 2023 14:30

fix(platform): fix unit tests fater refactorings

0ae7728

Merge remote-tracking branch 'origin/main' into feat/table-optimisation

2557586

N1XUS requested a review from fkolar June 13, 2023 12:36

fix(platform): remove unused component

0d898c8

N1XUS mentioned this pull request Jun 13, 2023

[Sourcing] fdp-table infinite scroll with multi selection rows #9988

Closed

mikerodonnell89 previously requested changes Jun 13, 2023

View reviewed changes

mikerodonnell89 reviewed Jun 13, 2023

View reviewed changes

libs/cdk/src/lib/utils/directives/selectable-list/selection.service.spec.ts Show resolved Hide resolved

fkolar approved these changes Jun 13, 2023

View reviewed changes

fix(platform): address pr comments

4263d33

N1XUS requested a review from mikerodonnell89 June 14, 2023 08:26

g-cheishvili approved these changes Jun 14, 2023

View reviewed changes

N1XUS added 2 commits June 14, 2023 13:42

fix(platform): address pr comments

223f6f7

Merge remote-tracking branch 'origin/main' into feat/table-optimisation

63ae765

N1XUS merged commit d2476c6 into main Jun 14, 2023

N1XUS deleted the feat/table-optimisation branch June 14, 2023 11:08

This was referenced Jan 5, 2024

[P2][Sourcing] fdp-table enableRowHeaderRole @input is missing in both 0.43.24 and 0.48.0 #11177

Closed

fix(platform): add enableRowHeaderRole input #9956

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(platform): table optimisation #9991

feat(platform): table optimisation #9991

N1XUS commented Jun 7, 2023 •

edited

Loading

netlify bot commented Jun 7, 2023 •

edited

Loading

github-actions bot commented Jun 7, 2023 •

edited

Loading

InnaAtanasova commented Jun 12, 2023

mikerodonnell89 left a comment

mikerodonnell89 commented Jun 13, 2023

g-cheishvili commented Jun 13, 2023

g-cheishvili commented Jun 13, 2023

N1XUS commented Jun 13, 2023

g-cheishvili commented Jun 13, 2023

N1XUS commented Jun 14, 2023

N1XUS commented Jun 14, 2023

N1XUS commented Jun 14, 2023

g-cheishvili left a comment

feat(platform): table optimisation #9991

feat(platform): table optimisation #9991

Conversation

N1XUS commented Jun 7, 2023 • edited Loading

Related Issue(s)

Description

Screenshots

Before:

After:

Please check whether the PR fulfills the following requirements

During Implementation

PR Quality

netlify bot commented Jun 7, 2023 • edited Loading

✅ Deploy Preview for fundamental-ngx ready!

github-actions bot commented Jun 7, 2023 • edited Loading

InnaAtanasova commented Jun 12, 2023

mikerodonnell89 left a comment

Choose a reason for hiding this comment

mikerodonnell89 commented Jun 13, 2023

g-cheishvili commented Jun 13, 2023

g-cheishvili commented Jun 13, 2023

N1XUS commented Jun 13, 2023

g-cheishvili commented Jun 13, 2023

N1XUS commented Jun 14, 2023

N1XUS commented Jun 14, 2023

N1XUS commented Jun 14, 2023

g-cheishvili left a comment

Choose a reason for hiding this comment

N1XUS commented Jun 7, 2023 •

edited

Loading

netlify bot commented Jun 7, 2023 •

edited

Loading

github-actions bot commented Jun 7, 2023 •

edited

Loading