- 
                Notifications
    You must be signed in to change notification settings 
- Fork 156
fix(igxGrid): Fix unmerge when merge groups partially overlap. #16363
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there is an issue:
- Open the performance sample with 1k records.
- Add sorting expression
    public sortingExpressions: ISortingExpression[] = [
        { fieldName: 'Name', dir: SortingDirection.Asc },
    ];
- Set cellMergeMode="always". Set[merge]=trueon all columns
- You get the following:
 5. Click on the row with ID 534. You will get the following:
5. Click on the row with ID 534. You will get the following:
 The two rows above the unmerged row are broken
The two rows above the unmerged row are broken
      
    | }; | ||
| // deep clone of inner map | ||
| for (const [key, value] of origRecord.cellMergeMeta) { | ||
| objCopy.cellMergeMeta.set(key, structuredClone(value)); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand why you clone the merged record, but why is the deep cloning at all? Isnt just changing the reference of the object enough? I dont think that using structuredClone for a grid with 1M records where a value object might be very nested is a good idea. Having said that, if you open the performance sample with 1M records, sort by name and unmerge the first row, the grid hangs and throws an error.
What if we just copy the object and the override the specific column. Something like:
if (unmergedRec.cellMergeMeta?.get(col.field)) {
                        // deep clone of object, since we don't want to pollute the original fully merged collection.
                        const objCopy = {
                            recordRef: origRecord.recordRef,
                            ghostRecord: origRecord.ghostRecord,
                            cellMergeMeta: new Map<string, IMergeByResult>(origRecord.cellMergeMeta.entries())
                        };
                  
                       // update copy with new meta from unmerged data record, but just for this column
                        objCopy.cellMergeMeta?.set(col.field, unmergedRec.cellMergeMeta.get(col.field));
                        result[index + i] = objCopy;
                    } else {
                        // this is the unmerged record, with no merge metadata
                        result[index + i] = unmergedRec;
                    }


Closes #16358
Closes #16361
Please also re-test: #16146
And also do more extensive testing with more complex merge groups and integration scenarios with other features.
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)