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

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

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

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

@Siemienik

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

@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;

This comment has been minimized.

Copy link
@Siemienik

Siemienik Feb 18, 2019

Member

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?

This comment has been minimized.

Copy link
@cxam

cxam Feb 20, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@Siemienik

Siemienik Feb 25, 2019

Member

I worry about case like this one #669

@guyonroche guyonroche merged commit d57d9c8 into exceljs:master Apr 11, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.