Conversation
|
|
|
Note that as part of the commits I |
|
It's quite interesting and promising, so far I replicated these changes in Slickgrid-Universal and gave it a try locally, with only 1 example, and it's quite cool to see in action. However, I'm not sure if it's possible at all, but from being an Excel user. When we make a larger selection then all is good, but when we take that large selection and want to shrink it then it's not behaving like Excel because in Excel it would shrink (as shown below) but in SlickGrid it seems to rather start a new selection which was a bit confusing at first. I understand that this might not be doable at all, but it's just the first observation that caught my eyes at first (see below). Apart from that, I haven't had a chance to look at the code that much, just some quick glance at it while implementing it in my own libs. I'll have to look at the code more closely when I get more time. Is there a reason why this was not created in 2 separate PRs? I mean there's Hybrid Selection Model and Drag-Fill, do they have to come together? Just curious... Also how does the drag-fill works? Because in Excel, it looks at the existing data to guess what the next cell values will be, is that what you wanted to do? Would the code implementation fall in the plugin or in the example? I like this new plugin and the hybrid mode, however I don't want to increase the lib too much so maybe that drag-fill is for example code?
|
|
It's clearer if you use the new CSS because the drag-fill border can be different to the original selection border. I created this in one update because there were a number of updates to core code that were for both and it was hard to separate them out, and last PR when I added functions to support future features you were asking 'why is that there?'. I could go back and do that if you need. Most of the changes for the HybridSelectionModel (other than the plugin itself) were just routing a few extra parameters through the event paths in the core code. The drag-fill function by default is in a DragHelper object in Slick.Core. Drag-fill has never been framed as a plugin, but with the new I did feel a little awkward in putting the default drag-fill function in core code, but I did think it was good to have something. We could just ship with nothing, and add the default implementation to the example as you suggest. I'd be fine with that. But I think everything else is worth adding - again, a lot of it is just adding extra params to event flows, plus some logic to support the drag-select as a first class mode as distinct from initial select. There were also some other minor fixes to annoying bugs where values entered into cells would disappear unpredictably. This was because the editor was not committing before the drag. This whole PR was really hard and time consuming to do, because it's all mouse based, so you have to log everything - no using debug tools. |
No no, it's all good, I was mainly asking out of curiosity to know if they were working hand in hand and it looks like they do.
Yeah I think that I would prefer that since, as you said, every users might have a different idea of what the drag-fill could and should do... so to add it in the example and let the user decide what logic he wants to follow is making more sense to me.
Yup, been there done that... I pretty much had to do the same when I replaced the jQuery mousewheel with our custom |
|
@ghiscoding the CI build seems to be bailing at a certain number of errors, so I fix what it lists and then get a new bunch. Is there a way to get it to list all the errors? I had a look at the CI build YML but it was not obvious. |
|
@ghiscoding looks like that's OK now. Also, I went to move the cell-copy code out of Even if someone was putting together their own custom copy code, I can see them using the helper functions. Opinions? |
I would say that if the utils are required for the feature to work then they're fine in Slick core. However if it's only useful or specific for the examples to work then that might be better to move them in the example(s)... I'm assuming that it's probably the first option, so in core seems fine to me. The only thing I would say is if there's any code that seems repetitive, then trying to merge some of the code might be good but only when possible. I'm not sure if the changes are related to the Cypress failing tests? I'm re-running the CI, we'll wait and see |
| this.init = function () { | ||
| navOnLR = args.grid.getOptions().editorCellNavOnLRKeys; | ||
| input = Slick.Utils.createDomElement('input', { type: 'text', className: 'editor-text' }, args.container); | ||
| input.addEventListener("keydown.nav", navOnLR ? handleKeydownLRNav : handleKeydownLRNoNav); |
There was a problem hiding this comment.
I'm trying to copy more of your code on my end and I found that this code isn't being executed at all because keydown.nav is not a valid JS event. For the history side of things, this kind of code came from when we were using jQuery which allowed passing namespace (e.g. keydown.suffix) but that won't work in pure JS since it only knows keydown and has no idea what keydown.nav is... so just removing the suffix should be good enough, even if in this case with/without the event doesn't seem to change much (do we even need that code at all?)
BTW, do you know what the editorCellNavOnLRKeys grid option does exactly? I never used it myself and don't have code that uses this option either..
| } | ||
|
|
||
|
|
||
| public static defaultCopyDraggedCellRange(e , args) { |
There was a problem hiding this comment.
| public static defaultCopyDraggedCellRange(e , args) { | |
| public static defaultCopyDraggedCellRange(_e: SlickEventData<any>, args: OnDragReplaceCellsEventArgs): void { |
| const dragReplaceEl = document.getElementById(this.id); | ||
| if (dragReplaceEl) { dragReplaceEl.remove(); } | ||
| //console.log('DragReplaceEl.removeEl'); |
There was a problem hiding this comment.
| const dragReplaceEl = document.getElementById(this.id); | |
| if (dragReplaceEl) { dragReplaceEl.remove(); } | |
| //console.log('DragReplaceEl.removeEl'); | |
| document.getElementById(this.id)?.remove(); |
| if (this.currentEditor) { | ||
| const commitSuccess = this.getEditorLock().commitCurrentEdit(); | ||
| if (!commitSuccess) { return false; } | ||
| } |
There was a problem hiding this comment.
| if (this.currentEditor) { | |
| const commitSuccess = this.getEditorLock().commitCurrentEdit(); | |
| if (!commitSuccess) { return false; } | |
| } | |
| if (this.currentEditor && !this.getEditorLock().commitCurrentEdit()) { | |
| return false; | |
| } |
|
@ghiscoding sorry this is taking a while. I went back to change the default drag-fill function and have found a reasonably complex bug that I need to fix. Been too busy last week or so. I'll take all the suggestions on board - they are not blocking the process. |
|
no problem at all, better take more time to get it right... Also the suggestions are just suggestions, you can click the "Commit Suggestion" if you want and that would push it as a commit. I merged only the 1 suggestion that fixed the Cypress failing tests (always glad to have so many tests, it often catches real issues as it did in this PR, hence the suggestion that I committed to fix it) I also added a bunch of Cypress tests on my end, I can copy them over in here too, that is when the implementation is finalized |
… drag-from selection rather than the cell opposite the drag-to cell
|
OK, this PR is now complete. I have added your suggestions and kept only helper methods in the core lib, the actual copy function has been shifted to the example. |
|
@6pac I had to compare against my own PR duplicate to find it, but yeah I found it and proposed and merged the commit suggestion and all Cypress tests are passing... Now I will try to update my own PR of the same feature but it might harder now that you did a force push which does rewrite history, but that's ok I'll figure it out :D However while looking at the CI running, I see that the build has a few typing issues (4x), you should think about fixing them before merging the PR. On my end, Perhaps you can change |
| return this._selectionMode; | ||
| } | ||
|
|
||
| setSelectionMode(mode: string) { |
There was a problem hiding this comment.
these 2 functions getSelectionMode() and setSelectionMode() aren't used, you can probably remove them?
| return this._currentlySelectedRange; | ||
| } | ||
|
|
||
| getPreviousRange() { |
|
Thanks, will check these out. The force push does rewrite history, but I'm not sure how or where, since my actual PR commits remain unchanged and the changes to master a applied 'silently'. This is way better than what I was doing before, but it's still not ideal. Let me know if you know of a better way. |
|
Yeah the git force is the way to go, it's all good I was able to replicate on my end. I purposely left 2 other PRs open to avoid conflicting with your PR, I'll merge them after your PR is merged. I try not to make too many git force push but when I do it via these 3 lines It's exciting to see this is going to get merged soon :) |
|
All good to merge? |
|
sure go ahead, though you might want to address the last comment I left here, otherwise it's all good, you can merge when you're ready. I'll merge my own PRs in a day or two. You should also merge this PR before your other small PR just to avoid merge conflicts, Cheers |
|
thanks. |
|
@6pac great, now I can proceed with some of my changes, I got couple more to do after that too and once I'm done I'll push a new release by the end of the week. Thanks |
|
@6pac can we renamed |
Take 2!!
New HybridSelectionModel combines cell and row selection models. It functions like CellSelectionModel except when certain conditions switch it into RowSelectionModel.
If the options.rowSelectOverride function exists, then HybridSelectionModel calls it expecting a boolean return to determine whether to behave like RowSelectionModel.
If this option is not set, then selection of a column that is a member of the array options.rowSelectColumnObjectArr will be treated with the behaviour of RowSelectionModel.
Drag and Replace option mirrors Excel as closely as possible. When a range of cells is selected, a grey box appears at the bottom right of the selection area, and dragging that grey box expands the selection area and replaces the newly selected cells will the contents of the existing cells. Excel specifically only allows expansion horizontally or vertically, but here we allow both. The current behaviour may be modified.
Note that the spreadsheet example the FormulaEditor causes problems with this new feature because it creates a new CellRangeSelector each time a cell is edited. This editor needs to add an event to the existing grid's CellRangeSelector, or create a new one.