-
Notifications
You must be signed in to change notification settings - Fork 6
Updated Excel edit style sample to functional component (React v19) #777
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: vnext
Are you sure you want to change the base?
Conversation
…o navigate correct
gridRef.current.markForCheck(); | ||
} else | ||
|
||
if (activeElem && activeElem.editMode && shouldAppendValue.current) { | ||
event.preventDefault(); | ||
activeElem.editValue = activeElem.editValue + event.key; | ||
shouldAppendValue.current = false; | ||
} | ||
} | ||
|
||
if ((key >= 48 && key <= 57) || (key >= 65 && key <= 90) || (key >= 97 && key <= 122)) { | ||
var columnName = grid.getColumnByVisibleIndex(activeElem.column).field; | ||
var cell = grid.getCellByColumn(activeElem.row, columnName); | ||
if (code === 'Backspace') { | ||
if(activeElem == null) { | ||
return; | ||
} | ||
const rowIndex = activeElem.row.index; | ||
const columnKey = activeElem.column.field; | ||
|
||
gridRef.current.data[rowIndex][columnKey] = ''; | ||
gridRef.current.markForCheck(); | ||
|
||
if (cell && !grid.crudService.cellInEditMode) { | ||
grid.crudService.enterEditMode(cell); | ||
cell.editValue = key; | ||
} | ||
} | ||
|
||
if (key == 13) { | ||
var thisRow = activeElem.row; | ||
var column = activeElem.column; | ||
var rowInfo = grid.dataView; | ||
if (code === 'Enter' || code === 'NumpadEnter') { | ||
|
||
if(activeElem == null) { | ||
return; | ||
} | ||
|
||
const thisRow = activeElem.row.index; | ||
const dataView = gridRef.current.dataView; | ||
const nextRowIndex = getNextEditableRowIndex(thisRow, dataView, event.shiftKey); | ||
|
||
gridRef.current.navigateTo(nextRowIndex, activeElem.column.visibleIndex, (obj: any) => { | ||
|
||
requestAnimationFrame(() => { | ||
gridRef.current.endEdit(true, obj); | ||
gridRef.current.clearCellSelection(); | ||
obj.target.activate(); | ||
|
||
}); | ||
}); | ||
|
||
var nextRow = this.getNextEditableRowIndex(thisRow, rowInfo, (args.detail.event as any).shiftKey); | ||
} | ||
}; | ||
|
||
grid.navigateTo(nextRow, column, (obj: any) => { | ||
obj.target.activate(); | ||
grid.clearCellSelection(); | ||
}); | ||
} | ||
} | ||
gridElement.addEventListener("keydown", handleKeyDown); |
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.
Multiple points:
- Why is this stuck in
useEffect
? The old sample usedgridKeydown
which should now beonGridKeydown
and should be used instead - What's with the
gridRef.current.markForCheck();
,requestAnimationFrame
andendEdit
calls? Those aren't needed in the original source (ng) and shouldn't be here as well. If they are, we might want to investigate why. Especially the rAF since it's in the already delayednavigateTo
callback
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.
afaik onGridKeyDown does not trigger when using any key, just the ones that are in navigation and have grid functions. Have this changed in 19? Initially I had it with onGridkeyDown but I remember having issues with this.
For the second one, the RAF is needed as the navigate is async, so I get timing issues and the cell does not exit edit mode in time, so this was the solution I came up with. Open to discussion to improve it as it does not feel very smooth
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.
Oh cool, just noticed that in https://www.infragistics.com/products/ignite-ui-angular/angular/components/grid/cell-editing#angular-grid-excel-style-editing-sample
Great, we made a sample that modifies navigation without our own dedicated event for that. Cool, cool 😶
Fine onKeyDown
then, at the very least still remove the useEffect
and use the default way to attach a handler:
That works just fine and is equivalent w/ less code. Also note, the correct arg type here is React.KeyboardEvent<IgrGrid>
instead of the lib.dom one and that means your grid ref is also:
const grid = event.currentTarget;
And that's both typed correctly and can replace the ref
entirely. Same goes for the gridEndEdit
, though you'll need to assert here. I'll leave a note there too and you can remove the ref entirely after.
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.
assigned keydown and added handler outside of useEffect
var code = event.code; | ||
var activeElem = gridRef.current.selectedCells[0]; |
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 const
instead of var
please :)
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.
removed
function gridEndEdit(event: IgrActiveNodeChangeEventArgs): void { | ||
gridRef.current.endEdit(true, event.detail); | ||
} |
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.
Note endEdit
is not correctly called with event.detail
, the second arg is for actual event and errors out. Also, no real public use should need it, so just replicate the original
function gridEndEdit(event: IgrActiveNodeChangeEventArgs): void { | |
gridRef.current.endEdit(true, event.detail); | |
} | |
function gridEndEdit(event: IgrActiveNodeChangeEventArgs): void { | |
const grid = event.currentTarget as IgrGrid; | |
grid.clearCellSelection(); | |
grid.endEdit(); | |
} |
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.
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.
Not really sure how to proceed with this one, advise please
…o navigate correct