Improve selection and paste

[velitasali]

Hi,

Fixes issue #419.

• [x] Fill data down or over.
• [x] Add a little indicator on the lower-right of the selection to be able to grab.
• [x] Show an indicator when the handle on the lower-right of the selection is grabbed and moved.
• [x] Ability to a register a function for how to replicate the data in the filled-to region.
• [x] A default way to show that it's implemented.

[ndrsn]

Hey @velitasali , thanks for this — looks great so far!

[velitasali]

@ndrsn Thanks. I might refactor these after the rest of the features is ready.

[velitasali]

Hi @ndrsn,

Could you look at the current state of MR and give feedback if possible? I think the first three should be ready, but I am unsure about what the last two items should do. The fourth commit should address the 4th item, but I might have understood it wrong.

Thanks.

[velitasali]

Btw, I put selection handle events into the self.mousemove to have the ability to autoscroll and not replicate, which needed to me to put a long if statement that disallows others functionalities that come before autoscroll. I am aware touchMove already replicates that behavior, so I was wondering your opinion on that. Should I add a new listener for mousemove and replicate, or should I keep it as it is?

[ndrsn]

Btw, I put selection handle events into the self.mousemove to have the ability to autoscroll and not replicate, which needed to me to put a long if statement that disallows others functionalities that come before autoscroll. I am aware touchMove already replicates that behavior, so I was wondering your opinion on that. Should I add a new listener for mousemove and replicate, or should I keep it as it is?

Not sure I follow (this isn't code I've ever touched myself) — and this doesn't seem to be showing in the diff just yet, I believe; do you have a line number where I can see the changes? Either way, I probably don't have a strong opinion about it. @hangxingliu is this something you've touched, or have an opinion about?

[hangxingliu]

Not sure I follow (this isn't code I've ever touched myself) — and this doesn't seem to be showing in the diff just yet, I believe; do you have a line number where I can see the changes? Either way, I probably don't have a strong opinion about it. @hangxingliu is this something you've touched, or have an opinion about?

I will take a look later and will give my opinion in here

[hangxingliu]

Hi, @ndrsn Could you grant this pull request the permission for running the workflow. So we can know can the new code pass the basic tests firstly.

[hangxingliu]

Hi @ndrsn ,I think @velitasali put the logic to individual function is good. This new function may can invoked from other event handler, so it can avoid long if statement and duplicate code.

https://github.com/TonyGermaneri/canvas-datagrid/pull/500/commits/042830fdc3128fb06632eee0a0ae37122e4a9612#diff-3f7a9934546ab5234148ac8c9d607d51b25c0fd156c9f0353d8b3f9d4385b74eR2220-R2270

self.selectionHandleMove = function (e) {

[velitasali]

This is almost done. I am going to write a few test cases, so I drafted this for now.

Btw, I realized I had misunderstood some things, so it took me a little to get things right. Now it should work similarly to Google Sheets. Feel free to review.

[velitasali]

Hi @ndrsn, one of the tests I added is failing, but it is unrelated to the MR and I thought I might get some help for it. Other than that, this is ready for a full review. Let me know what you think.

[velitasali]

eslint freaks out about property spread notation that I added. Is there a reason why that should happen?

[hangxingliu]

eslint freaks out about property spread notation that I added. Is there a reason why that should happen?

Hi @velitasali , I think you can just change the config of the eslint to fix it: changing the ecmaVersion to 12, like this:

{
  "root": true,
  "extends": [
    "eslint:recommended",
    "prettier"
  ],
  "parserOptions": {
    "ecmaVersion": 12,
    "sourceType": "module"
  },

[ndrsn]

eslint freaks out about property spread notation that I added. Is there a reason why that should happen?

I think the eslint config is out of date, apologies! I'll see if I can update it.

Update: what @hangxingliu said :-)

[velitasali]

Thanks, @hangxingliu and @ndrsn.

[velitasali]

Hi @ndrsn, I have pushed some new changes (not rebased yet), let me know what you think :)

[velitasali]

Here is the demo of this MR: https://codesandbox.io/s/datagrid-demo-v2w6j0?file=/demo.js

You can observe the changes by pasting data or dragging the handle. Also, the behavior can be configured by tweaking fillCellCallback inside demo.js.

[ndrsn]

Here is the demo of this MR: https://codesandbox.io/s/datagrid-demo-v2w6j0?file=/demo.js

Excellent, thank you so much — this makes testing so much easier.

I found a quirk or two. I'm not sure if it's a bug or just my expectation that's off:

When filling down, and then attempting to fill up with the unaltered selection — it fails. Not sure why. It also does not always seem to draw the overlay.

[velitasali]

I found a quirk or two. I'm not sure if it's a bug

Thanks, I was able to reproduced that and fixed it.

[velitasali]

Updated the demo as well.

[mdebrauw]

Hey @velitasali, this looks awesome, great work!

I have been doing some testing on the demo that you've posted and found an issue.

When I copy a single cell from an Excel or Google sheet to a single cell in CDG the value does not get copied. When copy/pasting a range of cells it works fine. Single cell copy/paste does work in the current version, so I think this PR is breaking that (maybe we need to add a test for this?).

Here's a gif which hopefully clarifies it:

[velitasali]

Hey @velitasali, this looks awesome, great work!

@ndrsn, thank you :)

When I copy a single cell from an Excel or Google sheet to a single cell in CDG the value does not get copied. When copy/pasting a range of cells it works fine. Single cell copy/paste does work in the current version, so I think this PR is breaking that (maybe we need to add a test for this?).

I was aware of this and forgot to bring it up, so thanks.

This issue is present in the master branch as well, which can be seen here: https://canvas-datagrid.js.org/demo.html. I thought I was using it wrong, so I didn't touch it. Let me know if I should fix it in this MR :)

[mdebrauw]

Hi @velitasali , hmm, you are right, this issue already exists. It doesn't happen consistently (not sure yet why). Is it a simple fix you think? Otherwise we can raise an issue and fix in separate PR.

[ndrsn]

Hi @velitasali , hmm, you are right, this issue already exists. It doesn't happen consistently (not sure yet why). Is it a simple fix you think? Otherwise we can raise an issue and fix in separate PR.

I'm keen on getting this merged, and if it's a bug that was already present and not directly related to the feature at hand, I say we fix this in a separate commit.

[ndrsn]

I think I've discovered another bug, when trying to make the selection smaller the selection seems 'stuck' at the larger size:

Contrast with Excel:

[velitasali]

Hi @velitasali , hmm, you are right, this issue already exists. It doesn't happen consistently (not sure yet why). Is it a simple fix you think? Otherwise we can raise an issue and fix in separate PR.

For reference, the offending line is here: https://github.com/TonyGermaneri/canvas-datagrid/blob/c4d405c88d71d5341e007c7c690a8bac3b68adf0/lib/events/index.js#L1966

What happens is, if there is only one cell to paste, it tries to do a simple paste (a guess). I think it is there for performance reasons, but I might be wrong, so I might need to know what that achieves to fix it.

I'm keen on getting this merged, and if it's a bug that was already present and not directly related to the feature at hand, I say we fix this in a separate commit.

:+1:

[velitasali]

I think I've discovered another bug, when trying to make the selection smaller the selection seems 'stuck' at the larger size

@ndrsn Interesting, I didn't realize that was an expected behavior. Currently, the code ignores if the overlay is in the selection bounds. That being said, I can implement this, but I need to know something.

Should we invoke the callback when we are shrinking, and what should we call behavior, still filling or something else?

[ndrsn]

Should we invoke the callback when we are shrinking, and what should we call behavior, still filling or something else?

Good questions — I just now tested this out in Google Sheets, and the behavior there seems different still:

In Google Sheets, using the handle only growing the selection is possible, not shrinking. Trying to shrink the selection does nothing, and does not clear the cells like in Excel.

So, considering the different implementations, I think for me it would not crucial that we support the Excel behavior in this PR: this PR implements filling, and the various edge cases are something we might tackle at a later date. That said, if we do want to support the Excel behavior, I feel like we should be able to configure it to support either the Excel or the Sheets behaviour. So an attribute like allowShrinkSelection would be needed, as would clearCellsWhenShrinkingSelection. I don't think the fill function needs to be called when shrinking the selection, because we only ever want to clear or not clear the cells — I can't think of a situation where you'd need to fill the cells being excluded from the selection. That's also an answer to your question regarding naming the behavior, I think: we can still call it 'filling'.

Note: naming of these attributes is still open for discussion, I just blurted out what first came to mind

[velitasali]

@ndrsn Could you test if it restores the previous data (I am unable to right now), like you overwrite a cell by moving over it and then shrinking. If it doesn't, it should be easy to implement.

So, considering the different implementations, I think for me it would not crucial that we support the Excel behavior in this PR: this PR implements filling, and the various edge cases are something we might tackle at a later date.

:+1:

So an attribute like allowShrinkSelection would be needed, as would clearCellsWhenShrinkingSelection.

So, these will remain unused for now, right?

[ndrsn]

So an attribute like allowShrinkSelection would be needed, as would clearCellsWhenShrinkingSelection.

So, these will remain unused for now, right?

Yeah those are only once we try to support both Excel and Sheets behavior.

[velitasali]

@ndrsn Hi again, I am going to add those attributes, but I was working on something else. I realized I can reset the direction once the user is in selection bounds, and it is working fine. Another thing I want to do is disallowing the auto-scroll for the opposite direction, and I think I have found a bug of some sort. Shouldn't auto-scroll not work if autoScrollOnMousemove is set to false (which is default?) https://github.com/TonyGermaneri/canvas-datagrid/blob/c4d405c88d71d5341e007c7c690a8bac3b68adf0/lib/events/index.js#L766

Right now, the else block completely overrides the behavior and allows auto-scroll even when it is disabled. The else block: https://github.com/TonyGermaneri/canvas-datagrid/blob/c4d405c88d71d5341e007c7c690a8bac3b68adf0/lib/events/index.js#L777

What is the expected behavior here? I want to fix this because I need to make some changes to it.

Edit

I read the documentation and now understand that is the expected behavior. I will make my changes accordingly.

[velitasali]

I pushed another commit. I didn't rebase it to make it easier to see the changes. If you prefer the other way around, let me know.

Also, I am not sure about allowShrinkSelection (now allowShrinkingSelection) and the other one, as I don't know how Excel behaves in that regard. Web version doesn't support it either (I can't Excel install right now).

[ndrsn]

@ndrsn Could you test if it restores the previous data (I am unable to right now), like you overwrite a cell by moving over it and then shrinking. If it doesn't, it should be easy to implement.

Sorry, missed this earlier — maybe you already figured it out, but just in case, here's what happens:

So it clears the cell values — no need to remember what the original values were — easier, I think?

[velitasali]

Sorry, missed this earlier — maybe you already figured it out, but just in case, here's what happens:

Thanks anyway. It is strange that it is erasing the data instead of undoing the fill, so that should be easier to implement (in another MR probably). Right now, I am making one last optimization as I am not satisfied with how the code finds the corners. It shouldn't take long.

Btw, I don't think the checks will pass unless I remove the object spread operator.

[ndrsn]

Btw, I don't think the checks will pass unless I remove the object spread operator.

I've updated the .eslintrc file in master — if you cherry-pick that commit (or just change 2017 to 2018), it should stop complaining (🤞)

[velitasali]

I've updated the .eslintrc file in master — if you cherry-pick that commit (or just change 2017 to 2018), it should stop complaining (crossed_fingers)

Oh. I didn't realize that. Thanks, will do!

[ndrsn]

Also, I am not sure about allowShrinkSelection (now allowShrinkingSelection) and the other one, as I don't know how Excel behaves in that regard. Web version doesn't support it either (I can't Excel install right now).

Are you not sure about the naming, or the behavior? I did wonder about your preference for continuous tense (shrinking) over present tense? I had to check the docs, seems like we've not been very consistent there, was wondering if you had an argument for one or the other?

[velitasali]

Are you not sure about the naming, or the behavior? I did wonder about your preference for continuous tense (shrinking) over present tense? I had to check the docs, seems like we've not been very consistent there, was wondering if you had an argument for one or the other?

I meant the behavior there. The video you sent cleared things up, so there is no problem now :)

[ndrsn]

@velitasali , this feels to me like it's ready to merge — great stuff! How about you?

[velitasali]

@velitasali , this feels to me like it's ready to merge — great stuff! How about you?

@ndrsn I think it is ready too. Minor issues (if there are any) can be tweaked with smaller commits. I think the features are complete :+1:

And, thank you for all the reviews/replies, @ndrsn :100:

[velitasali]

I am going to make one small change just so that when the user is in the selection bounds, we don't immediately get rid of the direction to preserve the overlay shape until the direction is recalculated. Nothing grand. Afterwards, it can be merged.

Edit

Pushed that now. It is ready.

[ndrsn]

Released in version 0.4.5!

🎉

Thanks a lot for all the hard work, @velitasali !