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.

Sign up for GitHub

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: some Date editor/filter improvements after new major version #1551

Merged
merged 5 commits into from
Jun 8, 2024
Merged

feat: some Date editor/filter improvements after new major version #1551

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8571348
fix: micro vs macro task for init rendering; properly hide editor
zewa666 May 28, 2024
476fc8d
feat: override default dataItemColumnValueExtractor editor beahvior
zewa666 May 28, 2024
99dd1ed
refactor: if dataItemColumnValueSetter returns true continue with def…
zewa666 May 30, 2024
31d5521
Merge remote-tracking branch 'upstream/master' into date-editor-impro…
zewa666 Jun 8, 2024
bab4891
refactor: apply review feedback
zewa666 Jun 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions examples/vite-demo-vanilla-bundle/src/examples/example19.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { type Column, type GridOption, SlickEventHandler, Editors } from '@slickgrid-universal/common';
import { type Column, type GridOption, SlickEventHandler, Editors, Formatters } from '@slickgrid-universal/common';
import { Slicker, type SlickVanillaGridBundle } from '@slickgrid-universal/vanilla-bundle';
import { ExampleGridOptions } from './example-grid-options';
import './example19.scss';
Expand Down Expand Up @@ -83,6 +83,16 @@ export default class Example19 {
}
];

this.columnDefinitions.push({
id: 'approvalDate',
name: 'Approval Date',
field: 'approvalDate',
minWidth: 120,
width: 120,
editor: { model: Editors.date, type: 'date'},
formatter: Formatters.dateIso,
exportWithFormatter: true
});
for (let i = 0; i < NB_ITEMS; i++) {
this.columnDefinitions.push({
id: i,
Expand All @@ -100,7 +110,7 @@ export default class Example19 {
return `${row + 1}:${cell + 1}`;
},
width: 60,
editor: { model: Editors.text }
editor: { model: Editors.text },
});
}

Expand Down Expand Up @@ -128,18 +138,22 @@ export default class Example19 {
onBeforePasteCell: (_e, args) => {
// deny the whole first row and the cells C-E of the second row
return !(args.row === 0 || (args.row === 1 && args.cell > 2 && args.cell < 6));
}
},
copyActiveEditorCell: true,
}
};
}

getData(itemCount: number) {
// mock a dataset
const datasetTmp: any[] = [];
const start = new Date(2000,0,1);
const end = new Date();
for (let i = 0; i < itemCount; i++) {
const d: any = (datasetTmp[i] = {});
d['id'] = i;
d['num'] = i;
d['approvalDate'] = new Date(start.getTime() + Math.random() * (end.getTime() - start.getTime()));
}

return datasetTmp;
Expand Down
20 changes: 12 additions & 8 deletions packages/common/src/editors/dateEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,8 @@ export class DateEditor implements Editor {
});
}

setTimeout(() => {
// INFO: Fixes issue no 2
Promise.resolve().then(() => {
Copy link
Owner

Choose a reason for hiding this comment

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

oh that's cool, does that do 1 CPU cycle? Because that's typically why I use setTimeout

Copy link
Contributor Author

@zewa666 zewa666 May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the event loop consists of cyclic tasks. Every such task is performing one render action. If you say setTimeout(..., 100) it skips 100ms and puts your task onto the macrotask queue.

There is a priority lane though, called microtask queue, which is a happening within every macrotask.
So all promises are pushed towards the end of a macrotask and executed all together before the next macrotask starts. Even nested promises.

Hence why often an optimization when working with DOM Elements is to stuff things into microtasks and only render them once alltogether in the next macrotask

Copy link
Owner

Choose a reason for hiding this comment

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

BTW, after reading the article it seems that we could instead use queueMicrotask(f) which seems shorter, isn't it better?

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 the same but yeah more expressive 👍

this.calendarInstance = new VanillaCalendar(this._inputElm, this._pickerMergedOptions);
this.calendarInstance.init();
if (!compositeEditorOptions) {
Expand All @@ -222,14 +223,17 @@ export class DateEditor implements Editor {
}

destroy() {
this.hide();
this._bindEventService.unbindAll();
this.calendarInstance?.destroy();
// INFO: Fixes issue no 3
Promise.resolve().then(() => {
this.hide();
this.calendarInstance?.destroy();
emptyElement(this._editorInputGroupElm);
emptyElement(this._inputElm);
this._editorInputGroupElm?.remove();
this._inputElm?.remove();
});

emptyElement(this._editorInputGroupElm);
emptyElement(this._inputElm);
this._editorInputGroupElm?.remove();
this._inputElm?.remove();
this._bindEventService.unbindAll();
}

clear() {
Expand Down
3 changes: 2 additions & 1 deletion packages/common/src/extensions/slickCellExcelCopyManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,9 @@ export class SlickCellExcelCopyManager {
// to decide if we evaluate the Formatter, we will use the same flag from Export which is "exportWithFormatter"
const activeCell = this._grid.getActiveCell();
const isActiveEditorCurrentCell = this._grid.getCellEditor() && activeCell?.row === row && activeCell?.cell === cell;
const copyActiveEditorCell = this.addonOptions?.copyActiveEditorCell || false;

if (!this.gridOptions.editable || !columnDef.editor || !isActiveEditorCurrentCell) {
if (!this.gridOptions.editable || !columnDef.editor || !isActiveEditorCurrentCell || copyActiveEditorCell) {
const isEvaluatingFormatter = (columnDef.exportWithFormatter !== undefined) ? columnDef.exportWithFormatter : (this.gridOptions.textExportOptions?.exportWithFormatter);
if (columnDef.formatter && isEvaluatingFormatter) {
const formattedOutput = columnDef.formatter(row, cell, item[columnDef.field], columnDef, item, this._grid);
Expand Down
7 changes: 7 additions & 0 deletions packages/common/src/interfaces/excelCopyBufferOption.interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ export interface ExcelCopyBufferOption<T = any> {
/** defaults to "copy-manager", sets the layer key for setting css values of copied cells. */
copiedCellStyleLayerKey?: string;

/**
* should copy the cells value on copy shortcut even when the editor is opened
*
* **NOTE**: affects only the default {@link ExcelCopyBufferOption#dataItemColumnValueExtractor}
* */
copyActiveEditorCell?: boolean;
Copy link
Owner

Choose a reason for hiding this comment

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

would there be a reason to NOT do that though? Do we really need to add this if it's expected to always do that? I remember that SlickGrid will first try to commit if an editor is opened before continuing, so I wonder if we need this extra arg?

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 a breaking change and we just had one recently ;)

besides, the reason for this could also be that it interfers with the original browser ctrl+c within the editor itself. e.g if it were a text editor and you merely select a part of it would you expect the part or the whole cell being copied?

Copy link
Owner

@ghiscoding ghiscoding May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I still don't understand why that is a breaking change? As for the browser ctrl+c, I assume that we have a preventDefault on the even that will not bubble up to the browser (if we don't, then we really should) and so it shouldn't interfere with the browser one. I would assume that it copies the whole cell content

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 what you mean but the default ctrl+c can never happen if there is that slickCellExternalCopyManager.handleKeyDown in place that circumvents and overrides the clipboard. And yep it does behave this way with my mentioned feature. So no matter whether you're in the cell or the editor it will always copy the cells content via dataItemColumnValueExtractor. As such it definitely feels like a breaking change for me if introduced as default.

Copy link
Owner

Choose a reason for hiding this comment

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

well ok, it doesn't matter much to me since I'm barely using that feature, so go ahead if you want a flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the amount of feature toggles is increasing indeed. 😅


/** option to specify a custom column value extractor function */
dataItemColumnValueExtractor?: (item: any, columnDef: Column<T>, row?: number, cell?: number) => string | HTMLElement | DocumentFragment | FormatterResultWithHtml | FormatterResultWithText | null;

Expand Down
24 changes: 12 additions & 12 deletions test/cypress/e2e/example19.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
describe('Example 19 - ExcelCopyBuffer with Cell Selection', () => {
const titles = [
'', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L',
'', 'Approval Date', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L',
'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y',
'Z', 'AA', 'AB', 'AC', 'AD', 'AE', 'AF', 'AG', 'AH', 'AI', 'AJ', 'AK'
];
Expand All @@ -16,7 +16,7 @@ describe('Example 19 - ExcelCopyBuffer with Cell Selection', () => {
.find('.slick-header-columns')
.children()
.each(($child, index) => {
if (index < titles.length) {
if (index > 0 && index < titles.length) {
expect($child.text()).to.eq(titles[index]);
}
});
Expand All @@ -28,15 +28,15 @@ describe('Example 19 - ExcelCopyBuffer with Cell Selection', () => {

describe('with Pagination of size 20', () => {
it('should click on cell B14 then Ctrl+Shift+End with selection B14-CV19', () => {
cy.getCell(14, 2, '', { parentSelector: '.grid19', rowHeight: GRID_ROW_HEIGHT })
cy.getCell(14, 3, '', { parentSelector: '.grid19', rowHeight: GRID_ROW_HEIGHT })
.as('cell_B14')
.click();

cy.get('@cell_B14')
.type('{ctrl}{shift}{end}');

cy.get('#selectionRange')
.should('have.text', '{"fromRow":14,"fromCell":2,"toRow":19,"toCell":100}');
.should('have.text', '{"fromRow":14,"fromCell":3,"toRow":19,"toCell":101}');
});

it('should click on cell CP3 then Ctrl+Shift+Home with selection A0-CP3', () => {
Expand Down Expand Up @@ -149,39 +149,39 @@ describe('Example 19 - ExcelCopyBuffer with Cell Selection', () => {
});

it('should click on cell E46 then Shift+End key with full row horizontal selection E46-CV46', () => {
cy.getCell(46, 5, '', { parentSelector: '.grid19', rowHeight: GRID_ROW_HEIGHT })
cy.getCell(46, 6, '', { parentSelector: '.grid19', rowHeight: GRID_ROW_HEIGHT })
.as('cell_E46')
.click();

cy.get('@cell_E46')
.type('{shift}{end}');

cy.get('#selectionRange')
.should('have.text', '{"fromRow":46,"fromCell":5,"toRow":46,"toCell":100}');
.should('have.text', '{"fromRow":46,"fromCell":6,"toRow":46,"toCell":101}');
});

it('should click on cell CP54 then Ctrl+Shift+End keys with selection E46-CV99', () => {
cy.getCell(54, 94, '', { parentSelector: '.grid19', rowHeight: GRID_ROW_HEIGHT })
cy.getCell(54, 95, '', { parentSelector: '.grid19', rowHeight: GRID_ROW_HEIGHT })
.as('cell_CP54')
.click();

cy.get('@cell_CP54')
.type('{ctrl}{shift}{end}');

cy.get('#selectionRange')
.should('have.text', '{"fromRow":54,"fromCell":94,"toRow":99,"toCell":100}');
.should('have.text', '{"fromRow":54,"fromCell":95,"toRow":99,"toCell":101}');
});

it('should click on cell CP95 then Ctrl+Shift+Home keys with selection C0-CP95', () => {
cy.getCell(95, 98, '', { parentSelector: '.grid19', rowHeight: GRID_ROW_HEIGHT })
cy.getCell(95, 99, '', { parentSelector: '.grid19', rowHeight: GRID_ROW_HEIGHT })
.as('cell_CP95')
.click();

cy.get('@cell_CP95')
.type('{ctrl}{shift}{home}');

cy.get('#selectionRange')
.should('have.text', '{"fromRow":0,"fromCell":0,"toRow":95,"toCell":98}');
.should('have.text', '{"fromRow":0,"fromCell":0,"toRow":95,"toCell":99}');
});

it('should click on cell CR5 again then Ctrl+Home keys and expect to scroll back to cell A0 without any selection range', () => {
Expand All @@ -204,15 +204,15 @@ describe('Example 19 - ExcelCopyBuffer with Cell Selection', () => {
});

it('should click on cell B14 then Shift+End with selection B14-24', () => {
cy.getCell(14, 2, '', { parentSelector: '.grid19', rowHeight: GRID_ROW_HEIGHT })
cy.getCell(14, 3, '', { parentSelector: '.grid19', rowHeight: GRID_ROW_HEIGHT })
.as('cell_B14')
.click();

cy.get('@cell_B14')
.type('{shift}{end}');

cy.get('#selectionRange')
.should('have.text', '{"fromRow":14,"fromCell":2,"toRow":14,"toCell":100}');
.should('have.text', '{"fromRow":14,"fromCell":3,"toRow":14,"toCell":101}');
});

it('should click on cell CS14 then Shift+Home with selection A14-CS14', () => {
Expand Down
Loading