(feat) Row Selection - Add in rowIndex to select output

[timwright35]

What kind of change does this PR introduce? (check one with "x")

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
Currently you have no clue what row index was on the selected row now that $$index is removed.

What is the new behavior?
This allows you to get the row index of the selected row when a row is selected.

Does this PR introduce a breaking change? (check one with "x")

• Yes
• No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

[wizarrc]

I originally thought it would be good to offer a helper function that one could call to determine the row index, but since that is more of an anti-pattern, I feel that it's better to change selected to be number[] instead. The user already has access to the rows array so it should be no problem, other than being a breaking change.

[wizarrc]

I don't like the suggested change for two reasons.

1. index is not the row index, but rather more like the current view index when scrollbarV is set to true.
2. rowIndex only returns one number, the view index that triggered the selection. If a keyboard button such as shift was pressed when activated, it would add multiple selections, yet only report one value (and not even accurate one).

My suggestion is to instead make the select event and selected input instead be of type number[] where it would return an array of row indexes and not the view index or even sorted index.

I spoke about my suggestion here in more detail.

References to show where index is taken from:
*ngFor="let row of temp; let i = index; trackBy: rowTrackingFn;"
(activate)="selector.onActivate($event, i)"
onActivate

[amcdnl]

Ya, I have to agree its kind of misleading. I think the best way would be to get the row index yourself. I think I know what you're trying to do, I'd suggest something like this:

function update(row) {
  // get the row
  const idx = this.rows.find(r => r.id === row.id);
  const row = this.rows[idx];

  // copy the row, do your updates to this new row item
  const newRow = {...row};

  // copy the array and insert new row in old ones place
  const rows = [...this.rows];
  rows.splice(idx, 1, newRow);

  // update rows
  this.rows = rows;
}

[timwright35]

Actually what I am doing is making it so when you select the row, output on table of (select), that it will change all the cells in the row to be editable at once, while leaving the other rows in the table alone. The issue is that before the returned row had that $$index in it so you could figure out which was selected, now it does not. This was a work around to make it so I could do similar.

Your this.rows.find would work if there was an id in the row data, currently there is not but it seems it would take more resources to do that since I might have to do the find on a large row array.

[timwright35]

1) index is not the row index, but rather more like the current view index when scrollbarV is set to true.

I did not look into the scrollbarV part, that is a good point.

2) rowIndex only returns one number, the view index that triggered the selection. If a keyboard button such as shift was pressed when activated, it would add multiple selections, yet only report one value (and not even accurate one).

Yea, I see what you mean on multiple select, that was not something I do in my code so I did not take it into account but it would not work in my change.

[wizarrc]

@amcdnl The problem seems to be that offering a row index service based on row output is an anti-pattern so the library does not provide one. Instead it's now on the burden of the user to know which index each row is on (assuming they don't sort). A way to solve that is for them to also create a Map<RowObject, number> of their rows collection, mapping the row to rowIndex themselves. They would also either have to enable external sorting and update the map themselves every time they want to sort, or disable sorting altogether. This seems like a anti-pattern as well as the user is having to duplicate code on their end because there is no other way for them to get the internal details the library has.

An alternative approach is either to replace selected of the row object type with a RowContext type that includes the rowIndex, row value, and maybe also include the sortedIndex and viewIndex. At a minimum what is needed is just a collection of selected indexes (rowIndex), because the user has access to the original rows source, they can index into it themselves.

[wizarrc]

In the demo provided in the library they have the Person type. Before the type included $$index forcefully and essentially mutated their type to something else. This was bad, so now the Person type is not mutated with internal properties as it was once before. Unfortunately it provided a service-like capability by linking the rowIndex to each object. That link in now done internally in a Map object but does not expose that mapping of any sort to the user. There does not appear to be any shortcut to either providing a service (anti-pattern) to bring back that knowledge to the user, forcing the user to recreate everything themselves (duplicate code), or change the meaning of Selected Input property (breaking change).

[wizarrc]

There is a 4th option of adding extra options to selected like {selected: any[], rowIndexes: number[]} which is a non-breaking change, and isn't an anti-pattern but a horribly API.

That could be achieved by having access the rowIndexes map (pass from the body to selection component as an input parameter) and doing something like this:

src/components/body/body.component.ts

...
<datatable-selection
    #selector
    [selected]="selected"
    [rows]="temp"
    [indexes]="rowIndexes" // Insert this here
...

src/components/body/selection.component.ts

@Input() indexes: number[]
...
const rowIndexes = selected.map(selectedRow => indexes.get(selectedRow)!);
this.select.emit({ 
    selected,
    rowIndexes
});

I think this is the quick hack fix you would be looking for, but it feels dirty and needing to be redesigned at some point.

[wizarrc]

@amcdnl Is rowIndexes map in the body component still a sorted row index map? If so then the above hack would break once the list is sorted internally without external sorting. That should at least be consistant with rowDetail context and bodyCell context where rowIndex is broken once sorted internally. Because the rows collection is a copy (immutable from the user perspective), the row index needs to match the user's row collection, not the internal one.

Here is why I think it's a sorted map and breaks for the user after sorting.
[rows]="_internalRows" is where the body component is passed in the after sorted internal rows copy.
getRowIndex(row: any): number shows:

getRowIndex(row: any): number {
    return this.rowIndexes.get(row);
}

where everything gets it's row indexes from.
this.rowIndexes.set(row, rowIndex); is where the row is assigned to the sorted row index.

[amcdnl]

@wizarrc - Yes, _internalRows is sorted array. So you're right that would not work for him either.

@Tempus35 - Is there any unique identifier for your row? Seems like there has to be something you can use for identification?

[timwright35]

@amcdnl There is not, at least not on all of the components. Basically what I have is what you have in the demo for editing cells. The difference is that we want the edit action to activate on a row click, not a cell, and for it to then make all the cells in that row editable at once. In your demo's case that isn't hard since you can get the rowIndex in the template for that cell and can make that cell editable. The only time you can not get the rowIndex is on the select action now.

The code I am working on is an older project that used a older datatable, till recently only minor changes were needed when upgraded but sadly the use of the $$index was used a lot in the code. I have made all the places within templates use rowIndex, which works great but the action to make the whole row turn to edit mode on a select fails without the index.

The PR was a means to make it so I have access to that rowIndex in select, admitting it is lacking in some use cases. I rather not have to make ids for all my row data and then try to use that with a find everytime a row is clicked, seems like it would be far more resources then just using an index that is given.

I will think more on this on my side, though the current PR as it is likely will not work.

[goldenis]

any plan to merge this PR? I need rowIndex via @output (select), as well.