When user press ctrl+a combination selection will be correct #78
Conversation
| var cellsWithData = { | ||
| right: tableData[selectedCell.row][selectedCell.col + 1] !== null, | ||
| left: tableData[selectedCell.row][selectedCell.col - 1] !== null, | ||
| top: tableData[selectedCell.row - 1][selectedCell.col] !== null, |
There was a problem hiding this comment.
How are you making sure the row index for top and column index for left doesn't become -1? If it's -1, you'll end up comparing undefined with null. Will this cause any problem?
|
|
||
| // If there is no true value in cellsWithData object we should select all table | ||
| if (countCellsWithData === 0) { | ||
| return {selectAll: true}; |
There was a problem hiding this comment.
| return {selectAll: true}; | |
| return { selectAll: true }; |
|
|
||
| return dataAt; | ||
| if (tableData[selectedCell.row][selectedCell.col] !== null) { | ||
| return {cellWithData: true}; |
There was a problem hiding this comment.
| return {cellWithData: true}; | |
| return { cellWithData: true }; |
| bottom: false, | ||
| all: false, | ||
| hasData: false | ||
| function nearestCellsWithData(selectedCell, tableData) { |
There was a problem hiding this comment.
| function nearestCellsWithData(selectedCell, tableData) { | |
| function getAdjacentCellsWithData(selectedCell, tableData) { |
There was a problem hiding this comment.
Based on the explanation, I'd call this adjacentCellHasData().
| right: tableData[selectedCell.row][selectedCell.col + 1] !== null, | ||
| left: tableData[selectedCell.row][selectedCell.col - 1] !== null, | ||
| top: tableData[selectedCell.row - 1][selectedCell.col] !== null, | ||
| bottom: tableData[selectedCell.row + 1][selectedCell.col] !== null, |
There was a problem hiding this comment.
Add a current: tableData[selectedCell.row][selectedCell.col] !== null, then just return the cellsWithData object. In fact, it doesn't really a var declared.
The reason for this is that the function will do exactly what the name suggests, and does not add logic on top to dictate what other functions should do with the information. The caller of this function can then decide under what scenario it should select all etc.
| } | ||
| } | ||
| // Return false in the case when we need to select all table data | ||
| if (nearestDataAt.selectAll) { |
There was a problem hiding this comment.
Move the for (var key in ...) logic from inside the getAdjacentCellsWithData() function to here.
| * @returns {Array} - coordinats that needs to be selected. Example of the returned data: [[startRow, startCol, endRow, endCol]] | ||
| * @param {array} tableData - an array of data from the Handsontable library | ||
| * @param {int} startSearchFrom - index of the row/col from what we should start a search | ||
| * @param {int} colIndex - a not necessary option that tells us what we need to look, a row index or column index |
There was a problem hiding this comment.
Am I right in suggesting this?
| * @param {int} colIndex - a not necessary option that tells us what we need to look, a row index or column index | |
| * @param {int} colIndex - (optional) when provided, the search is performed along the rows for the column specified |
|
|
||
| firstCol = firstCol || 0; | ||
| // If column index is not 0 it means that data what we need to select is in the next column | ||
| firstCol = firstCol === 0 ? firstCol : firstCol + 1; |
There was a problem hiding this comment.
If I'm reading this right, if firstCol is 1, you'd end up adding 1 to it and make it 2? What's the reason behind doing this?
There was a problem hiding this comment.
@tonytlwu it would be easier to explain on an example:
How selection will behave without this line.
https://share.getcloudapp.com/yAu2Pode
And with this line.
https://share.getcloudapp.com/WnuG1woX
This behavior occurs because of how we searching for the coordinates.
Our functions getStartIndex and getEndIndex are looking for the null in value of the cell to determine that there is no data in that cell. That is why they are returning an index of the cell with null, but we are interested to select only cells with data, that is why we adding to the row or cell 1 here.
This behavior occurs only in the situation I showed in the example when there is more that 1 data selection available in the data source.
There was a problem hiding this comment.
Could getStartIndex() and getEndIndex() change its output so that it returns the first/last cell with data instead of without data? It would match the function names better.
| } | ||
| } | ||
| // If row index is not 0 it means that data what we need to select is in the next row | ||
| firstRow = firstRow === 0 ? firstRow : firstRow + 1; |
There was a problem hiding this comment.
Same as above. Why are we turning 1 to 2 while 0 remains the same as 0?
| } | ||
| } | ||
| lastCol = getEndIndex(tableData[firstRow], firstCol); | ||
| lastCol = lastCol < selectedCellPosition.col ? selectedCellPosition.col : lastCol; |
There was a problem hiding this comment.
| lastCol = lastCol < selectedCellPosition.col ? selectedCellPosition.col : lastCol; | |
| lastCol = Math.min(lastCol, selectedCellPosition.col); |
There was a problem hiding this comment.
@tonytlwu, in this case, we should use Math.max as well. Because Math.min will cause this issue https://share.getcloudapp.com/wbu0AeAb
There was a problem hiding this comment.
Yes, indeed. It should be Math.max().
|
@YaroslavOvdii @sofiiakvasnevska What's happening with this? Looks like it's been put on pause. Can we continue with this? |
@sofiiakvasnevska
Issue
https://github.com/Fliplet/fliplet-studio/issues/6108
Description
When user press ctrl+a combination selection will be correct
Screenshots/screencasts
https://share.getcloudapp.com/OAub0Y6m
https://share.getcloudapp.com/Z4uLbO18 - large data selection 60k rows ~100 cols
Backward compatibility
This change is fully backward compatible.
Reviewers
@upplabs-alex-levchenko @MaksymShokin