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

Allow adding new rows or columns on paste #553

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

romanstetsyk
Copy link
Contributor

@romanstetsyk romanstetsyk commented Apr 6, 2023

Fixes issue #552.

Changes proposed in this pull request:

  • Add a new attribute, allowGridSizeChangeOnPaste (which is false by default). If it's set to true and pasted data is bigger than the grid, new rows or columns will be created as needed. Please see the screen recording.
Screen.Recording.2023-04-06.at.23.05.47.mov

This is slow for big datasets because a) .addRow() calls .refresh() b) on each cell pasted .selectCell() is called. So there's plenty of room for optimization.

Copy link
Collaborator

@ndrsn ndrsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @romanstetsyk, thanks so much! I have a few minor points, but I really appreciate the tests you've written in addition to the new functionality — really makes things easier to merge :)

lib/defaults.js Outdated Show resolved Hide resolved
lib/docs.js Outdated Show resolved Hide resolved
lib/events/index.js Outdated Show resolved Hide resolved
];
const newColSchema = {
...lastColSchema,
name: `col${self.schema.length + 1}:${Date.now()}`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not be using self.getSchema() here? Or rather:

const schema = self.getSchema();

if (!schema[columnIndex]) {
  if (self.attributes.allowGridSizeChangeOnPaste) {
    const lastColSchema = schema[self.orders.columns[schema.length - 1]];
  }
  const newColSchema = {
    ...lastColSchema,
    name: `col${schema.length + 1}${Date.now()}`,
    title: ' ',
  };
  self.addColumn(newColSchema)
}

Also, two (three?) other points:

  1. can we safely assume that the last column's schema is similar to the newly pasted column? What is you're pasting text and the previous existing column schema defines it as type: number ?
  2. What is the purpose of the name including the current date? Do we even need to name the new column?
  3. Why the empty title?

@romanstetsyk romanstetsyk requested a review from ndrsn April 13, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants