Nothing Special   »   [go: up one dir, main page]

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

Fix existing row styles when using spliceRows #737

Merged
merged 2 commits into from
Apr 11, 2019

Conversation

cxam
Copy link
Contributor
@cxam cxam commented Feb 3, 2019

This PR fixes the style issues reported in #305 and #697 to copy over any existing styles already applied to rows. I have also included additional tests for this fix.

@alubbe
Copy link
Member
alubbe commented Feb 11, 2019

I'm really not familiar with how styles work, but is this related to #722 @Siemienik ?

@Siemienik
Copy link
Member

@alubbe it is quite different issue

#722 prevent about duplicating references when reading.

@@ -330,6 +330,10 @@ Worksheet.prototype = {
rSrc = this._rows[i - 1];
if (rSrc) {
this.getRow(i + nExpand).values = rSrc.values;
// eslint-disable-next-line no-loop-func
rSrc.eachCell({ includeEmpty: true }, function(cell, colNumber) {
this.getRow(i + nExpand).getCell(colNumber).style = cell.style;
Copy link
Member

Choose a reason for hiding this comment

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

hmm.. have we 100% that cell.style object hasn't multiple references from some different cells?
may should we use deep copy here?

@cxam @alubbe what are You thinking about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Siemienik do you think that would be necessary because the row that we get the style for each cell gets set to undefined anyway?

Copy link
Member

Choose a reason for hiding this comment

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

I worry about case like this one #669

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.

4 participants