feat: Composite Editor should support CellSelectionModel

[wscherphof]

As discussed in #1987, for the Composite Editor:

• Allow either the CellSelectionModel or the RowSelectionModel;
• Don't break the selected cell ranges when using the CellSelectionModel;
• Provide an extra callback onDispose, that always gets called when the editor is removed.

[bolt-new-by-stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.0%. Comparing base (72b56aa) to head (d97b8bd).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1990   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         195      195           
  Lines       35921    35931   +10     
  Branches    10648    10651    +3     
=======================================
+ Hits        35921    35931   +10     

Flag	Coverage Δ
angular	100.0% <ø> (ø)
universal	100.0% <100.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ghiscoding]

it looks like all Cypress E2E tests are failing for all frameworks on the same Example 30 test...

We also need unit test to cover the new event, you could maybe add a spy to the onDispose on any of the test. I think a good place to put it might in be in this test below, you could add the onDispose check as well and an expect that it was called

slickgrid-universal/packages/composite-editor-component/src/slick-composite-editor.component.spec.ts

Lines 695 to 702 in 44a7420

	it('should NOT execute "onClose" callback when user confirms the closing of the modal when "onClose" callback is defined', () => {
	const mockProduct = { id: 222, address: { zip: 123456 }, productName: 'Product ABC', price: 12.55 };
	vi.spyOn(gridStub, 'getDataItem').mockReturnValue(mockProduct);
	const cancelSpy = vi.spyOn(gridStub.getEditController() as any, 'cancelCurrentEdit');
	const mockOnClose = vi.fn();
	const mockModalOptions = { headerTitle: 'Details', modalType: 'edit', onClose: mockOnClose } as CompositeEditorOpenDetailOption;
	const spyOnClose = vi.spyOn(mockModalOptions, 'onClose');

[wscherphof]

it looks like all Cypress E2E tests are failing for all frameworks on the same Example 30 test...

We also need unit test to cover the new event, you could maybe add a spy to the onDispose on any of the test. I think a good place to put it might in be in this test below, you could add the onDispose check as well and an expect that it was called

slickgrid-universal/packages/composite-editor-component/src/slick-composite-editor.component.spec.ts

Lines 695 to 702 in 44a7420

	it('should NOT execute "onClose" callback when user confirms the closing of the modal when "onClose" callback is defined', () => {
	const mockProduct = { id: 222, address: { zip: 123456 }, productName: 'Product ABC', price: 12.55 };
	vi.spyOn(gridStub, 'getDataItem').mockReturnValue(mockProduct);
	const cancelSpy = vi.spyOn(gridStub.getEditController() as any, 'cancelCurrentEdit');
	const mockOnClose = vi.fn();
	const mockModalOptions = { headerTitle: 'Details', modalType: 'edit', onClose: mockOnClose } as CompositeEditorOpenDetailOption;
	const spyOnClose = vi.spyOn(mockModalOptions, 'onClose');

New test for onDispose in 4800075.

E2E: removed the check on "no row selected after mass-selection" in fc28c61.
EDIT: I found some other instances now; see d97b8bd, where I also set the selectedRanges to [] when modalType = mass-update, because the selection has no meaning to the editor in that case.

[ghiscoding]

E2E: removed the check on "no row selected after mass-selection" in fc28c61.
EDIT: I found some other instances now; see d97b8bd, where I also set the selectedRanges to [] when modalType = mass-update, because the selection has no meaning to the editor in that case.

Yeah ok I did see your change and I agree on the Mass Update to remove all selections, but I wonder if removing all the E2E tests that were previously expecting the row selection to be cleared is the correct behavior. I mean, I wonder if your change is correct or if we should rather add a new option flag for this? I'm saying this because you're changing the behavior so in theory it's a slight breaking change.

@zewa666 you're also using the Composite Editor right? This PR would include a behavior change since it would now reapply the Row Selection that the user had before opening the Modal window. That's a behavior change because at the moment, when Mass Selection changes are applied and the user clicks Save, it currently clears the Row Selection... so I'm wondering if we should add a new flag to let the user decide on the behavior. What do you think? Like a clearSelectionOnSave or something

below is the current behavior

[zewa666]

no we don't use the composite editor since our requirements are to show related grids, hence we went with using the column definitions and so on but building a custom form with ngxformly with those.

I think the proposed change here is good, and while its a breaking change I feel like its actually more of a fix. I dont see a use case why the selection should be cleared. so I'd say its good 👍

[ghiscoding]

alright thanks for the feedback, let's merge as it is then. Thanks a lot for the contribution, expect a new release over the weekend :)