Conversation
luka-nextcloud
requested review from
max-nextcloud,
mejo- and
silverkszlo
as code owners
🖼️ Screenshotsdemo.webm |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@luka-nextcloud Thanks a lot! That looks really great. I'll take a look at the code. Could you rebase and Thanks! |
max-nextcloud
left a comment
max-nextcloud
left a comment
There was a problem hiding this comment.
All in all looks good.
I think it would be nice to not move the cursor around to change the sort order. I added some more detailed comments explaining how I think this would be feasible.
| const selectHeaderCell = (editor, targetIndex = 0) => { | ||
| let currentIndex = 0 | ||
| let selectionPos | ||
| editor.state.doc.descendants((node, pos) => { | ||
| if (node.type.name !== 'tableHeader') { | ||
| return true | ||
| } | ||
| if (currentIndex === targetIndex) { | ||
| selectionPos = pos + 1 | ||
| return false | ||
| } | ||
| currentIndex += 1 | ||
| return true | ||
| }) | ||
| if (selectionPos != null) { | ||
| editor.commands.setTextSelection(selectionPos) | ||
| } | ||
| return selectionPos | ||
| } |
There was a problem hiding this comment.
This should not need to set the text selection but you can use it to get the header cell position to provite to the command.
| return true | ||
| }, | ||
| sortColumn: | ||
| (direction = 'asc', explicitColumnIndex = null) => |
There was a problem hiding this comment.
It would be nice if this command did not depend on the cursor position. This would remove the need to move the cursor around just to change the sorting of a table.
So instead of getting the $cell from the state below we'd need to provide it as a function argument. I think the easiest would be to just provide the position of the cell as returned by this.getPos() inside the cell node and then find the cell from there.
This would also remove the need for the explicitColumnIndex - as the index would just be the index of the cell identified by the position.
Signed-off-by: Luka Trovic <luka@nextcloud.com>
luka-nextcloud
force-pushed
the
sort-by-column
branch
from
87911cd to
d5f42ce
Compare
2 participants
📝 Summary
🚧 TODO
🏁 Checklist
npm run lint/npm run stylelint/composer run cs:check)