Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handling paste data when it is bigger than the number of cells #236

Closed
TikiTDO opened this issue Sep 29, 2023 · 8 comments
Closed

Handling paste data when it is bigger than the number of cells #236

TikiTDO opened this issue Sep 29, 2023 · 8 comments
Labels
major change This issue requires major change in grid's functionality, API, structure, etc.

Comments

@TikiTDO
Copy link
Contributor

TikiTDO commented Sep 29, 2023

Describe the bug

There is no way to deal with a user pasting in more data than the sheet has grid rows

Current behavior

Currently, when a sheet receives a paste event, the pasteData handler discards any rows and columns which do not have a corresponding grid cell without any way to know that data was discarded

Expected behavior

It would be great to have an option to receive the rows/columns that were unable to be parsed in some way. The best place for me to deal with it would be in the onCellsChanged somehow, perhaps as another argument, but if that's not possible an extra callback like like onExtraPasteData or something to that effect would allow some way to handle this.

@DLowHP DLowHP added the major change This issue requires major change in grid's functionality, API, structure, etc. label Oct 2, 2023
@DLowHP
Copy link
Collaborator

DLowHP commented Oct 2, 2023

This is related with #206 - as I said there we'll take this into consideration in the upcoming 5th release.

@TikiTDO
Copy link
Contributor Author

TikiTDO commented Oct 2, 2023

If you're looking for capacity to do it I can throw something together if I know which approach your team would accept. I've been digging around the internals of this project for a client feature, so it shouldn't be too hard if I have a design goal

@DLowHP
Copy link
Collaborator

DLowHP commented Oct 2, 2023

So you're thinking about adding this to v4 version? If so you can create a POC and pull request so that I can take a look at your proposal. I'm thinking this could be added as an extra ReactGrid callback property (see: PublicModel.ts) - just as you said (it's important though to keep backwards compatibility). I'm having trouble coming up with a clear naming but I think it should be something like onPasteOverflow - suggesting that the pasted data overflows current grid structure, what do you think?

@TikiTDO
Copy link
Contributor Author

TikiTDO commented Oct 2, 2023

The version of the code in develop's what I've been reading, so it's the one I'm familiar with. I glanced at the v5-dev branch, but it seems you're just starting on that so I don't know if you'll want an external contributor running around trying to help with that.

onPasteOverflow seems like a better name than what I proposed. I'll poke at it to see if I can make it work with my scenario, and if the idea works then I can open an PR and see above helping migrate it to v5.

@DLowHP
Copy link
Collaborator

DLowHP commented Oct 4, 2023

That's okay. Yeah, v5 is just starting, I'm at the stage where everything is experimental - it's like going in circles, back and forth - so it would be hard to cooperate on this but at the same time if you have any ideas how we could improve you're free to tell us about them.

Cool! For now you can create it for v4.x and for v5 we'll integrate it with time.

@DLowHP
Copy link
Collaborator

DLowHP commented Oct 4, 2023

Hi @TikiTDO! I've spoken with PO and we agreed that it shouldn't be handled in another callback and rather the paste function should be able to manage this. This also means that this should be opt-in not opt-out - if the user needs to handle data overflow they should "plug" their own paste callback.

@TikiTDO
Copy link
Contributor Author

TikiTDO commented Oct 4, 2023

@DLowHP Yeah, that's the direction my thought process has been going too. The latest idea I had last night was some way to hook into the row/cell iteration here:

    rows.forEach((row, ri) =>
      row.forEach((cell, ci) => {
        const rowIdx = activeSelectedRange.first.row.idx + ri;
        const columnIdx = activeSelectedRange.first.column.idx + ci;
        if (
          rowIdx <= cellMatrix.last.row.idx &&
          columnIdx <= cellMatrix.last.column.idx
        ) {
          lastLocation = cellMatrix.getLocation(rowIdx, columnIdx);
          state = tryAppendChangeHavingGroupId(
            state,
            lastLocation,
            cell
          ) as State;
        }
      })
    );

My biggest issue thus far is that I'm also using the undo implementation with useState and I haven't come up with a way to integrate the two. I'll let it roll around in the back of my mind for a bit longer, I'm working on other features at the moment. Hopefully by the time I'm back to this there will be things I want to try.

@MichaelMatejko
Copy link
Contributor

The general approach would be to handle the paste event outside of the ReactGrid component. Just use onPasteCapture and stopPropagation on a parent component. Then parse the pasted data and rebuild the CellMatrix on demand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major change This issue requires major change in grid's functionality, API, structure, etc.
Projects
None yet
Development

No branches or pull requests

3 participants