From 33aca6f9c2742a2fe549d6288042904266c8d4ad Mon Sep 17 00:00:00 2001 From: Ghislain Beaulac Date: Fri, 4 Sep 2020 15:46:23 -0400 Subject: [PATCH 1/2] fix(editors): fix all Editors custom validators with invalid results --- src/app/examples/grid-editor.component.ts | 10 +++++++++- .../editors/__tests__/selectEditor.spec.ts | 6 ++---- .../angular-slickgrid/editors/selectEditor.ts | 16 ++++++---------- .../angular-slickgrid/models/locale.interface.ts | 2 +- .../styles/slickgrid-theme-material.scss | 2 +- .../styles/slickgrid-theme-salesforce.scss | 2 +- 6 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/app/examples/grid-editor.component.ts b/src/app/examples/grid-editor.component.ts index f0f0ff188..55cf3b48d 100644 --- a/src/app/examples/grid-editor.component.ts +++ b/src/app/examples/grid-editor.component.ts @@ -244,6 +244,12 @@ export class GridEditorComponent implements OnInit { }, params: { formatters: [Formatters.collectionEditor, Formatters.percentCompleteBar], + }, + validator: (value, args) => { + if (value < 50) { + return { valid: false, msg: 'Please use at least 50%' }; + } + return { valid: true, msg: '' }; } }, { id: 'start', @@ -582,7 +588,9 @@ export class GridEditorComponent implements OnInit { } onValidationError(e, args) { - alert(args.validationResults.msg); + if (args.validationResults) { + alert(args.validationResults.msg); + } } changeAutoCommit() { diff --git a/src/app/modules/angular-slickgrid/editors/__tests__/selectEditor.spec.ts b/src/app/modules/angular-slickgrid/editors/__tests__/selectEditor.spec.ts index 886e2d9ff..b64c6ae2b 100644 --- a/src/app/modules/angular-slickgrid/editors/__tests__/selectEditor.spec.ts +++ b/src/app/modules/angular-slickgrid/editors/__tests__/selectEditor.spec.ts @@ -428,15 +428,13 @@ describe('SelectEditor', () => { mockItemData = { id: 1, gender: '', isActive: true }; mockColumn.internalColumnEditor.required = true; gridOptionMock.autoCommitEdit = true; - const commitEditSpy = jest.spyOn(gridStub.getEditorLock(), 'commitCurrentEdit'); - const commitChangeSpy = jest.spyOn(editorArguments, 'commitChanges'); + const spy = jest.spyOn(gridStub.getEditorLock(), 'commitCurrentEdit'); editor = new SelectEditor(editorArguments, true); editor.loadValue(mockItemData); editor.save(); - expect(commitEditSpy).not.toHaveBeenCalled(); - expect(commitChangeSpy).not.toHaveBeenCalled(); + expect(spy).not.toHaveBeenCalled(); }); }); diff --git a/src/app/modules/angular-slickgrid/editors/selectEditor.ts b/src/app/modules/angular-slickgrid/editors/selectEditor.ts index 8813f8f9e..3ac007632 100644 --- a/src/app/modules/angular-slickgrid/editors/selectEditor.ts +++ b/src/app/modules/angular-slickgrid/editors/selectEditor.ts @@ -417,16 +417,12 @@ export class SelectEditor implements Editor { } save() { - // autocommit will not focus the next editor - const validation = this.validate(); - if (validation && validation.valid && this.isValueChanged()) { - if (!this._destroying && this.hasAutoCommitEdit) { - // do not use args.commitChanges() as this sets the focus to the next row. - // also the select list will stay shown when clicking off the grid - this.grid.getEditorLock().commitCurrentEdit(); - } else { - this.args.commitChanges(); - } + if (!this._destroying && this.hasAutoCommitEdit) { + // do not use args.commitChanges() as this sets the focus to the next row. + // also the select list will stay shown when clicking off the grid + this.grid.getEditorLock().commitCurrentEdit(); + } else { + this.args.commitChanges(); } } diff --git a/src/app/modules/angular-slickgrid/models/locale.interface.ts b/src/app/modules/angular-slickgrid/models/locale.interface.ts index 0d6703cf1..544b9e365 100644 --- a/src/app/modules/angular-slickgrid/models/locale.interface.ts +++ b/src/app/modules/angular-slickgrid/models/locale.interface.ts @@ -92,7 +92,7 @@ export interface Locale { /** Text "Remove Sort" shown in Header Menu */ TEXT_REMOVE_SORT: string; - /** Text "Cancel" shown in the Long Text Editor dialog */ + /** Text "Save" shown in the Long Text Editor dialog */ TEXT_SAVE: string; /** Text "Select All" displayed in the Multiple Select Editor/Filter */ diff --git a/src/app/modules/angular-slickgrid/styles/slickgrid-theme-material.scss b/src/app/modules/angular-slickgrid/styles/slickgrid-theme-material.scss index 1fe7f9c70..3019c5957 100644 --- a/src/app/modules/angular-slickgrid/styles/slickgrid-theme-material.scss +++ b/src/app/modules/angular-slickgrid/styles/slickgrid-theme-material.scss @@ -11,5 +11,5 @@ @import './flatpickr.min'; @import './multiple-select'; - @import './slick-without-bootstrap-min-styling'; @import './slickgrid-theme-material.lite'; + @import './slick-without-bootstrap-min-styling'; diff --git a/src/app/modules/angular-slickgrid/styles/slickgrid-theme-salesforce.scss b/src/app/modules/angular-slickgrid/styles/slickgrid-theme-salesforce.scss index 980a96adb..6cff66e41 100644 --- a/src/app/modules/angular-slickgrid/styles/slickgrid-theme-salesforce.scss +++ b/src/app/modules/angular-slickgrid/styles/slickgrid-theme-salesforce.scss @@ -11,5 +11,5 @@ @import './flatpickr.min'; @import './multiple-select'; -@import './slick-without-bootstrap-min-styling'; @import './slickgrid-theme-salesforce.lite'; +@import './slick-without-bootstrap-min-styling'; From d4adacc0607f32402aadd8d50b891eed351d0025 Mon Sep 17 00:00:00 2001 From: Ghislain Beaulac Date: Tue, 8 Sep 2020 10:27:21 -0400 Subject: [PATCH 2/2] fix(editors): all Editors should call commitChanges even when invalid - for the Editor validators to work properly it needs to call the commitChanges() at all time even when potentially invalid or value is unchanged so that the validator can be executed at all time --- src/app/examples/grid-editor.component.ts | 14 +++++++------- .../editors/__tests__/floatEditor.spec.ts | 4 ++-- .../editors/__tests__/sliderEditor.spec.ts | 4 ++-- .../editors/autoCompleteEditor.ts | 14 ++++++++------ .../angular-slickgrid/editors/checkboxEditor.ts | 8 +++++++- .../angular-slickgrid/editors/dateEditor.ts | 15 ++++++++------- .../angular-slickgrid/editors/floatEditor.ts | 14 ++++++++------ .../angular-slickgrid/editors/integerEditor.ts | 14 ++++++++------ .../angular-slickgrid/editors/longTextEditor.ts | 2 ++ .../angular-slickgrid/editors/selectEditor.ts | 5 ++++- .../angular-slickgrid/editors/sliderEditor.ts | 14 ++++++++------ .../angular-slickgrid/editors/textEditor.ts | 14 ++++++++------ 12 files changed, 72 insertions(+), 50 deletions(-) diff --git a/src/app/examples/grid-editor.component.ts b/src/app/examples/grid-editor.component.ts index 55cf3b48d..962e4412e 100644 --- a/src/app/examples/grid-editor.component.ts +++ b/src/app/examples/grid-editor.component.ts @@ -245,12 +245,12 @@ export class GridEditorComponent implements OnInit { params: { formatters: [Formatters.collectionEditor, Formatters.percentCompleteBar], }, - validator: (value, args) => { - if (value < 50) { - return { valid: false, msg: 'Please use at least 50%' }; - } - return { valid: true, msg: '' }; - } + // validator: (value, args) => { + // if (value < 50) { + // return { valid: false, msg: 'Please use at least 50%' }; + // } + // return { valid: true, msg: '' }; + // } }, { id: 'start', name: 'Start', @@ -266,7 +266,7 @@ export class GridEditorComponent implements OnInit { sortable: true, type: FieldType.date, editor: { - model: Editors.date + model: Editors.date, }, }, { id: 'finish', diff --git a/src/app/modules/angular-slickgrid/editors/__tests__/floatEditor.spec.ts b/src/app/modules/angular-slickgrid/editors/__tests__/floatEditor.spec.ts index 141fc4c68..39bdb6688 100644 --- a/src/app/modules/angular-slickgrid/editors/__tests__/floatEditor.spec.ts +++ b/src/app/modules/angular-slickgrid/editors/__tests__/floatEditor.spec.ts @@ -405,7 +405,7 @@ describe('FloatEditor', () => { expect(spy).toHaveBeenCalled(); }); - it('should not call anything when the input value is not a valid float number', () => { + it('should call "commitCurrentEdit" even when the input value is not a valid float number', () => { mockItemData = { id: 1, price: null, isActive: true }; gridOptionMock.autoCommitEdit = true; const spy = jest.spyOn(gridStub.getEditorLock(), 'commitCurrentEdit'); @@ -415,7 +415,7 @@ describe('FloatEditor', () => { editor.setValue('-.'); editor.save(); - expect(spy).not.toHaveBeenCalled(); + expect(spy).toHaveBeenCalled(); }); it('should call "getEditorLock" and "save" methods when "hasAutoCommitEdit" is enabled and the event "focusout" is triggered', (done) => { diff --git a/src/app/modules/angular-slickgrid/editors/__tests__/sliderEditor.spec.ts b/src/app/modules/angular-slickgrid/editors/__tests__/sliderEditor.spec.ts index 887dd9144..762c71091 100644 --- a/src/app/modules/angular-slickgrid/editors/__tests__/sliderEditor.spec.ts +++ b/src/app/modules/angular-slickgrid/editors/__tests__/sliderEditor.spec.ts @@ -389,7 +389,7 @@ describe('SliderEditor', () => { expect(spy).toHaveBeenCalled(); }); - it('should not call anything when the input value is the same as the default value', () => { + it('should call "commitCurrentEdit" even when the input value is the same as the default value', () => { mockItemData = { id: 1, price: 0, isActive: true }; gridOptionMock.autoCommitEdit = true; const spy = jest.spyOn(gridStub.getEditorLock(), 'commitCurrentEdit'); @@ -398,7 +398,7 @@ describe('SliderEditor', () => { editor.loadValue(mockItemData); editor.save(); - expect(spy).not.toHaveBeenCalled(); + expect(spy).toHaveBeenCalled(); }); it('should call "getEditorLock" and "save" methods when "hasAutoCommitEdit" is enabled and the event "focusout" is triggered', (done) => { diff --git a/src/app/modules/angular-slickgrid/editors/autoCompleteEditor.ts b/src/app/modules/angular-slickgrid/editors/autoCompleteEditor.ts index c2d320a28..26becf308 100644 --- a/src/app/modules/angular-slickgrid/editors/autoCompleteEditor.ts +++ b/src/app/modules/angular-slickgrid/editors/autoCompleteEditor.ts @@ -212,12 +212,14 @@ export class AutoCompleteEditor implements Editor { save() { const validation = this.validate(); - if (validation && validation.valid && this.isValueChanged()) { - if (this.hasAutoCommitEdit) { - this.grid.getEditorLock().commitCurrentEdit(); - } else { - this.args.commitChanges(); - } + const isValid = (validation && validation.valid) || false; + + if (this.hasAutoCommitEdit && isValid) { + // do not use args.commitChanges() as this sets the focus to the next row. + // also the select list will stay shown when clicking off the grid + this.grid.getEditorLock().commitCurrentEdit(); + } else { + this.args.commitChanges(); } } diff --git a/src/app/modules/angular-slickgrid/editors/checkboxEditor.ts b/src/app/modules/angular-slickgrid/editors/checkboxEditor.ts index 8bccdc12c..b1dc25054 100644 --- a/src/app/modules/angular-slickgrid/editors/checkboxEditor.ts +++ b/src/app/modules/angular-slickgrid/editors/checkboxEditor.ts @@ -118,8 +118,14 @@ export class CheckboxEditor implements Editor { save() { const validation = this.validate(); - if (validation && validation.valid && this.isValueChanged() && this.hasAutoCommitEdit) { + const isValid = (validation && validation.valid) || false; + + if (this.hasAutoCommitEdit && isValid) { + // do not use args.commitChanges() as this sets the focus to the next row. + // also the select list will stay shown when clicking off the grid this.grid.getEditorLock().commitCurrentEdit(); + } else { + this.args.commitChanges(); } } diff --git a/src/app/modules/angular-slickgrid/editors/dateEditor.ts b/src/app/modules/angular-slickgrid/editors/dateEditor.ts index 34f1e2133..ef07e8364 100644 --- a/src/app/modules/angular-slickgrid/editors/dateEditor.ts +++ b/src/app/modules/angular-slickgrid/editors/dateEditor.ts @@ -220,14 +220,15 @@ export class DateEditor implements Editor { } save() { - // autocommit will not focus the next editor const validation = this.validate(); - if (validation && validation.valid && this.isValueChanged()) { - if (this.hasAutoCommitEdit) { - this.grid.getEditorLock().commitCurrentEdit(); - } else { - this.args.commitChanges(); - } + const isValid = (validation && validation.valid) || false; + + if (this.hasAutoCommitEdit && isValid) { + // do not use args.commitChanges() as this sets the focus to the next row. + // also the select list will stay shown when clicking off the grid + this.grid.getEditorLock().commitCurrentEdit(); + } else { + this.args.commitChanges(); } } diff --git a/src/app/modules/angular-slickgrid/editors/floatEditor.ts b/src/app/modules/angular-slickgrid/editors/floatEditor.ts index 5c3e495a7..81320bb49 100644 --- a/src/app/modules/angular-slickgrid/editors/floatEditor.ts +++ b/src/app/modules/angular-slickgrid/editors/floatEditor.ts @@ -160,12 +160,14 @@ export class FloatEditor implements Editor { save() { const validation = this.validate(); - if (validation && validation.valid && this.isValueChanged()) { - if (this.hasAutoCommitEdit) { - this.grid.getEditorLock().commitCurrentEdit(); - } else { - this.args.commitChanges(); - } + const isValid = (validation && validation.valid) || false; + + if (this.hasAutoCommitEdit && isValid) { + // do not use args.commitChanges() as this sets the focus to the next row. + // also the select list will stay shown when clicking off the grid + this.grid.getEditorLock().commitCurrentEdit(); + } else { + this.args.commitChanges(); } } diff --git a/src/app/modules/angular-slickgrid/editors/integerEditor.ts b/src/app/modules/angular-slickgrid/editors/integerEditor.ts index 56a4bb0de..6ecaff817 100644 --- a/src/app/modules/angular-slickgrid/editors/integerEditor.ts +++ b/src/app/modules/angular-slickgrid/editors/integerEditor.ts @@ -129,12 +129,14 @@ export class IntegerEditor implements Editor { save() { const validation = this.validate(); - if (validation && validation.valid && this.isValueChanged()) { - if (this.hasAutoCommitEdit) { - this.grid.getEditorLock().commitCurrentEdit(); - } else { - this.args.commitChanges(); - } + const isValid = (validation && validation.valid) || false; + + if (this.hasAutoCommitEdit && isValid) { + // do not use args.commitChanges() as this sets the focus to the next row. + // also the select list will stay shown when clicking off the grid + this.grid.getEditorLock().commitCurrentEdit(); + } else { + this.args.commitChanges(); } } diff --git a/src/app/modules/angular-slickgrid/editors/longTextEditor.ts b/src/app/modules/angular-slickgrid/editors/longTextEditor.ts index 48493c204..da51870da 100644 --- a/src/app/modules/angular-slickgrid/editors/longTextEditor.ts +++ b/src/app/modules/angular-slickgrid/editors/longTextEditor.ts @@ -210,6 +210,8 @@ export class LongTextEditor implements Editor { const isValid = (validation && validation.valid) || false; if (this.hasAutoCommitEdit && isValid) { + // do not use args.commitChanges() as this sets the focus to the next row. + // also the select list will stay shown when clicking off the grid this.grid.getEditorLock().commitCurrentEdit(); } else { this.args.commitChanges(); diff --git a/src/app/modules/angular-slickgrid/editors/selectEditor.ts b/src/app/modules/angular-slickgrid/editors/selectEditor.ts index 3ac007632..262db03ae 100644 --- a/src/app/modules/angular-slickgrid/editors/selectEditor.ts +++ b/src/app/modules/angular-slickgrid/editors/selectEditor.ts @@ -417,7 +417,10 @@ export class SelectEditor implements Editor { } save() { - if (!this._destroying && this.hasAutoCommitEdit) { + const validation = this.validate(); + const isValid = (validation && validation.valid) || false; + + if (!this._destroying && this.hasAutoCommitEdit && isValid) { // do not use args.commitChanges() as this sets the focus to the next row. // also the select list will stay shown when clicking off the grid this.grid.getEditorLock().commitCurrentEdit(); diff --git a/src/app/modules/angular-slickgrid/editors/sliderEditor.ts b/src/app/modules/angular-slickgrid/editors/sliderEditor.ts index e93c195eb..7013ae7c4 100644 --- a/src/app/modules/angular-slickgrid/editors/sliderEditor.ts +++ b/src/app/modules/angular-slickgrid/editors/sliderEditor.ts @@ -153,12 +153,14 @@ export class SliderEditor implements Editor { save() { const validation = this.validate(); - if (validation && validation.valid && this.isValueChanged()) { - if (this.hasAutoCommitEdit) { - this.args.grid.getEditorLock().commitCurrentEdit(); - } else { - this.args.commitChanges(); - } + const isValid = (validation && validation.valid) || false; + + if (this.hasAutoCommitEdit && isValid) { + // do not use args.commitChanges() as this sets the focus to the next row. + // also the select list will stay shown when clicking off the grid + this.grid.getEditorLock().commitCurrentEdit(); + } else { + this.args.commitChanges(); } } diff --git a/src/app/modules/angular-slickgrid/editors/textEditor.ts b/src/app/modules/angular-slickgrid/editors/textEditor.ts index 1b22b1005..e6c229c55 100644 --- a/src/app/modules/angular-slickgrid/editors/textEditor.ts +++ b/src/app/modules/angular-slickgrid/editors/textEditor.ts @@ -129,12 +129,14 @@ export class TextEditor implements Editor { save() { const validation = this.validate(); - if (validation && validation.valid && this.isValueChanged()) { - if (this.hasAutoCommitEdit) { - this.grid.getEditorLock().commitCurrentEdit(); - } else { - this.args.commitChanges(); - } + const isValid = (validation && validation.valid) || false; + + if (this.hasAutoCommitEdit && isValid) { + // do not use args.commitChanges() as this sets the focus to the next row. + // also the select list will stay shown when clicking off the grid + this.grid.getEditorLock().commitCurrentEdit(); + } else { + this.args.commitChanges(); } }