T1280020 - DataGrid - The 'row' parameter in the FocusedRowChanged event refers to a non-focused row if the grid height is small#30678
Conversation
| key: any, | ||
| ): DeferredObj<number | unknown> { | ||
| if (!isDefined(key)) { | ||
| this._resetFocusedRow(); |
There was a problem hiding this comment.
Question
Is _resetFocusedRow() sync? If not and there are some async ops inside, won't it be misleading that we don't wait for it?
|
|
||
| public _navigateToRow(key, needFocusRow?) { | ||
| public _navigateToRow( | ||
| key: any, |
There was a problem hiding this comment.
Minor
Afair, there was some export type Key = any
It will not change anything for TS, but let's still use it for better readability
| this.option('focusedRowIndex', -1); | ||
| } | ||
| return this._navigateToRow(key); | ||
| return this._navigateToRow(key, false); |
There was a problem hiding this comment.
Question
Why? Isn't it required to focus row when autoNavigateToFocusedRow is true?
There was a problem hiding this comment.
It is just a refactoring, I've added this argument to make method typing easier.
I think a row can be focused only if the row focusing is enabled. Also this method is related to the scrolling and can be used without focusing.
|
|
||
| private _focusRowByKey(key) { | ||
| private _focusRowByKey( | ||
| key: any, |
There was a problem hiding this comment.
Minor
Let's use Key type (see another comment below)
| focusedRowEnabled: true, | ||
| onFocusedRowChanged(e) { | ||
| const data = e.row?.data; | ||
| $('#otherContainer').text(data.id); |
There was a problem hiding this comment.
Why do we need a container to output data? Can't we remember the row object in the window? And check it as a whole to make sure it's valid?
There was a problem hiding this comment.
It is more easier way for me. Surely you can use the window object but it will require more code,
There was a problem hiding this comment.
Well, okay, let's leave it as is.
| public _navigateToRow(key, needFocusRow?) { | ||
| public _navigateToRow( | ||
| key: Key, | ||
| needFocusRow: boolean, |
There was a problem hiding this comment.
I suggest setting this argument to a default value of false right away
There was a problem hiding this comment.
Why? It is a required argument now and its value defined at calling the method. Here is passing false.
There was a problem hiding this comment.
Yeah, I'd like to suggest leaving this argument optional with a default value of false. That way you don't have to explicitly pass false when calling this method.
But I won't insist on this.
…ent refers to a non-focused row if the grid height is small (DevExpress#30678)
…ent refers to a non-focused row if the grid height is small (DevExpress#30678)
3 participants
No description provided.