Skip to content
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

feat: notify onValidationError on paste if validation failed #1462

Merged
merged 9 commits into from
Apr 15, 2024
12 changes: 10 additions & 2 deletions examples/vite-demo-vanilla-bundle/src/examples/example22.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ import {
} from '@slickgrid-universal/common';
import { SlickCustomTooltip } from '@slickgrid-universal/custom-tooltip-plugin';
import { Slicker, type SlickVanillaGridBundle } from '@slickgrid-universal/vanilla-bundle';
import { ExampleGridOptions } from './example-grid-options';
import { BindingEventService } from '@slickgrid-universal/binding';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like separating external vs internal imports, so would you mind moving this import further to the top? Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about a linter rule for that? ;)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh does that actually exists? Dang ESLint is so powerful :P

Copy link
Contributor Author

@zewa666 zewa666 Apr 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create an additional PR after this one as it currently would involve 319 problems, pretty much all auto-fixable though ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its not only separation but also unifies blank lines between those and ordered imports. So with the extra PR you get a chance to tweak those configs as you prefer.

I'll set it up tomorrow as I need to get some rest today


import { ExampleGridOptions } from './example-grid-options';
import './example22.scss';
import type { TranslateService } from '../translate.service';

Expand All @@ -25,11 +26,13 @@ export default class Example22 {
fetchResult = '';
statusClass = 'is-success';
statusStyle = 'display: none';
private _bindingEventService: BindingEventService;

constructor() {
this.translateService = (<any>window).TranslateService;
this.selectedLanguage = this.translateService.getCurrentLanguage();
this.selectedLanguageFile = `${this.selectedLanguage}.json`;
this._bindingEventService = new BindingEventService();
}

attached() {
Expand All @@ -44,10 +47,15 @@ export default class Example22 {
{ ...ExampleGridOptions, ...this.gridOptions },
this.dataset
);

this._bindingEventService.bind(document.querySelector(`.grid1`)!, 'onvalidationerror', (event) =>
alert((event as CustomEvent)?.detail.args.validationResults.msg)
);
}

dispose() {
this.sgb?.dispose();
this._bindingEventService.unbindAll();
}

/* Define grid Options and Columns */
Expand Down Expand Up @@ -80,7 +88,7 @@ export default class Example22 {
minWidth: 100,
filterable: true,
type: FieldType.number,
editor: { model: Editors.text },
editor: { model: Editors.text, validator: (val) => (val > 100 ? { msg: 'Max 100% allowed', valid: false} : { msg: '', valid: true}) },
},
{
id: 'start',
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
"@types/node": "^20.12.7",
"cross-env": "^7.0.3",
"cypress": "^13.7.3",
"cypress-real-events": "^1.12.0",
"dotenv": "^16.4.5",
"eslint": "^9.0.0",
"eslint-plugin-cypress": "^2.15.1",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const mockGetSelectionModel = {
const returnValueStub = jest.fn();
const gridStub = {
getActiveCell: jest.fn(),
getActiveCellNode: jest.fn(),
getColumns: jest.fn().mockReturnValue([
{ id: 'firstName', field: 'firstName', name: 'First Name', },
{ id: 'lastName', field: 'lastName', name: 'Last Name' },
Expand All @@ -47,6 +48,7 @@ const gridStub = {
triggerEvent: jest.fn().mockReturnValue({ getReturnValue: returnValueStub }),
onCellChange: new SlickEvent(),
onKeyDown: new SlickEvent(),
onValidationError: new SlickEvent(),
} as unknown as SlickGrid;

const mockCellSelectionModel = {
Expand All @@ -67,6 +69,7 @@ const mockTextEditor = {
applyValue: jest.fn(),
loadValue: jest.fn(),
serializeValue: jest.fn(),
validate: jest.fn().mockReturnValue({ valid: true, msg: null }),
} as unknown as InputEditor;

const mockTextEditorImplementation = jest.fn().mockImplementation(() => mockTextEditor);
Expand All @@ -80,9 +83,9 @@ describe('CellExternalCopyManager', () => {
lastNameElm.textContent = 'Last Name';
const mockEventCallback = () => { };
const mockColumns = [
{ id: 'firstName', field: 'firstName', name: 'First Name', editor: Editors.text, editorClass: Editors.text },
{ id: 'firstName', field: 'firstName', name: 'First Name', editor: { model: Editors.text }, editorClass: Editors.text },
{ id: 'lastName', field: 'lastName', name: lastNameElm, },
{ id: 'age', field: 'age', name: 'Age', editor: Editors.text, editorClass: Editors.text },
{ id: 'age', field: 'age', name: 'Age', editor: { model: Editors.text }, editorClass: Editors.text },
] as Column[];
let plugin: SlickCellExternalCopyManager;
const gridOptionsMock = {
Expand Down Expand Up @@ -196,6 +199,23 @@ describe('CellExternalCopyManager', () => {
expect(applyValSpy).toHaveBeenCalledWith(mockItem, 'some value');
});

it('should call "setDataItemValueForColumn" and expect an onValidationError triggered if validation failed', () => {
const validationResults = { valid: false, msg: 'foobar' };
const applyValSpy = jest.spyOn(mockTextEditor, 'applyValue');
const loadValSpy = jest.spyOn(mockTextEditor, 'loadValue');
const validationSpy = jest.spyOn(mockTextEditor, 'validate').mockReturnValue(validationResults);
jest.spyOn(gridStub, 'getSelectionModel').mockReturnValue(mockCellSelectionModel as any);
const notifySpy = jest.spyOn(gridStub.onValidationError, 'notify');
const mockItem = { firstName: 'John', lastName: 'Doe' };
plugin.init(gridStub);
plugin.setDataItemValueForColumn(mockItem, mockColumns[0], 'some value');

expect(loadValSpy).toHaveBeenCalledWith(mockItem);
expect(applyValSpy).toHaveBeenCalledWith(mockItem, 'some value');
expect(validationSpy).toHaveBeenCalled();
expect(notifySpy).toHaveBeenCalledWith(expect.objectContaining({ validationResults }));
});

it('should call "setDataItemValueForColumn" and expect item last name to change with new value when no Editor is provided', () => {
const mockItem = { firstName: 'John', lastName: 'Doe' };
plugin.init(gridStub);
Expand Down
28 changes: 21 additions & 7 deletions packages/common/src/extensions/slickCellExternalCopyManager.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { createDomElement, getHtmlStringOutput, stripTags } from '@slickgrid-universal/utils';

import type { Column, ExcelCopyBufferOption, ExternalCopyClipCommand, OnEventArgs } from '../interfaces/index';
import type { Column, Editor, ExcelCopyBufferOption, ExternalCopyClipCommand, OnEventArgs } from '../interfaces/index';
import { SlickEvent, SlickEventData, SlickEventHandler, type SlickGrid, SlickRange, type SlickDataView, Utils as SlickUtils } from '../core/index';

// using external SlickGrid JS libraries
Expand Down Expand Up @@ -127,15 +127,15 @@ export class SlickCellExternalCopyManager {

// if a custom getter is not defined, we call serializeValue of the editor to serialize
if (columnDef) {
if (columnDef.editor) {
if (columnDef.editorClass) {
const tmpP = document.createElement('p');
const editor = new (columnDef as any).editor({
const editor = new (columnDef as any).editorClass({
container: tmpP, // a dummy container
column: columnDef,
event,
position: { top: 0, left: 0 }, // a dummy position required by some editors
grid: this._grid,
});
}) as Editor;
editor.loadValue(item);
retVal = editor.serializeValue();
editor.destroy();
Expand All @@ -155,15 +155,29 @@ export class SlickCellExternalCopyManager {
}

// if a custom setter is not defined, we call applyValue of the editor to unserialize
if (columnDef.editor) {
if (columnDef.editorClass) {
const tmpDiv = document.createElement('div');
const editor = new (columnDef as any).editor({
const editor = new (columnDef as any).editorClass({
container: tmpDiv, // a dummy container
column: columnDef,
position: { top: 0, left: 0 }, // a dummy position required by some editors
grid: this._grid
});
}) as Editor;
Copy link
Owner

@ghiscoding ghiscoding Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zewa666 ahh sorry, I missed this earlier, I think we can remove the any on the column and make the editorClass parsed as EditorConstructor that was added not long ago. I'm guessing we might have to explicitly parse it though

const editor = new (columnDef.editorClass as EditorConstructor)({ })`

unless this other suggestion works, which would be better

const editor = new columnDef.editorClass({ }) as EditorConstructor`

Note that there's 2 of these to change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if thats gonna work as editorClass can be both the ctor and an editor according to the type

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah ok I'll take a look at that later, I wanted to check on Stackblitz but it looks like it's blocked by the VPN on work laptop, so anyway I can review that on a separate PR if it could be improved (or not)

editor.loadValue(item);
const validationResults = editor.validate(undefined, value);
if (!validationResults.valid) {
const activeCell = this._grid.getActiveCell()!;
this._grid.onValidationError.notify({
editor,
cellNode: this._grid.getActiveCellNode()!,
validationResults,
row: activeCell?.row,
cell: activeCell?.cell,
column: columnDef,
grid: this._grid,
});
}

editor.applyValue(item, value);
editor.destroy();
tmpDiv.remove();
Expand Down
11 changes: 11 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 18 additions & 1 deletion test/cypress/e2e/example22.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,23 @@ describe('Example 22 - Row Based Editing', () => {
cy.get('.slick-cell').first().should('have.class', 'slick-rbe-unsaved-cell');
});

it('should stay in editmode if saving failed', () => {
it('should fire onvalidationerror event when pasting and resulting in invalid validation result', (done) => {
cy.reload();

cy.get('.action-btns--edit').first().click();

cy.get('.slick-cell.l1.r1').first().click().type('120{enter}');
cy.get('.slick-cell.l1.r1').first().click().realPress(['Control', 'C']);

cy.on('window:alert', (str) => {
expect(str).to.equal('Max 100% allowed');
done();
});
cy.get('.slick-cell.l2.r2').first().click().realPress(['Control', 'V']);
cy.get('.slick-cell.active').type('{enter}');
});

it('should stay in editmode if saving failed', (done) => {
cy.reload();

cy.get('.action-btns--edit').first().click();
Expand All @@ -74,6 +90,7 @@ describe('Example 22 - Row Based Editing', () => {
cy.on('window:confirm', () => true);
cy.on('window:alert', (str) => {
expect(str).to.equal('Sorry, 40 is the maximum allowed duration.');
done();
});

cy.get('.slick-row.slick-rbe-editmode').should('have.length', 1);
Expand Down
1 change: 1 addition & 0 deletions test/cypress/support/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
// -- This will overwrite an existing command --
// Cypress.Commands.overwrite("visit", (originalFn, url, options) => { ... })
import '@4tw/cypress-drag-drop';
import 'cypress-real-events';
import { convertPosition } from './common';

declare global {
Expand Down
3 changes: 2 additions & 1 deletion test/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
],
"types": [
"jest",
"node"
"node",
"cypress-real-events"
],
"allowJs": true,
"skipLibCheck": true,
Expand Down
Loading