Skip to content

FIX assignStyle on Table.store #1907

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.

Sign up for GitHub

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

Jump to bottom
Open
o100ja wants to merge 2 commits into
base: master
Choose a base branch
from
Open

FIX assignStyle on Table.store #1907

o100ja wants to merge 2 commits into from
+1 −1

Conversation

@o100ja
Copy link

@o100ja o100ja commented Nov 17, 2021

The current implementation sets any style properties directly to the cell instead of merging into the existing cell style.

The result is that if you set a style in a column of a Table model it will break the resulting xlsx

Summary

I need to be able to set the numFmt style on a Worksheet table. If I do it direct as in this example:

  ws.addTable({
    name     : "FlexReport",
    ref      : "A1",
    headerRow: true,
    totalsRow: true,
    style    : {
      theme         : "TableStyleMedium9",
      showRowStripes: true,
    },
    columns  : [{
      key         : "id",
      name        : "ID",
      filterButton: true,
    }, {
      key         : "name",
      name        : "Name",
      filterButton: true,
    }, {
      key         : "value",
      name        : "value",
      filterButton: true,
      style       : {numFmt: "#,#00.00"},
    }],
    rows,
  });

the exported xlsx file is broke with the following error:
image

Test plan

After reviewing the source code I've tried to do the same export with the following change, which produced the correct format:

  ws.addTable({
    name     : "FlexReport",
    ref      : "A1",
    headerRow: true,
    totalsRow: true,
    style    : {
      theme         : "TableStyleMedium9",
      showRowStripes: true,
    },
    columns  : [{
      key         : "id",
      name        : "ID",
      filterButton: true,
    }, {
      key         : "name",
      name        : "Name",
      filterButton: true,
    }, {
      key         : "value",
      name        : "value",
      filterButton: true,
      style       : {style:{numFmt: "#,#00.00"}}, // <-- Note the change which works but will overwrite any existing cell.style
    }],
    rows,
  });

Related to source code (for typings update)

@o100ja
FIX assignStyle on Table.store
5b41d7a 
The current implementation sets any style properties directly to the cell instead of merging into the existing cell style.

The result is that if you set a style in a column of a Table model it will break the resulting `xlsx`
@Siemienik Siemienik self-requested a review November 17, 2021 23:35
Siemienik
Siemienik reviewed Nov 18, 2021
View reviewed changes
Copy link
Member

@Siemienik Siemienik left a comment

Choose a reason for hiding this comment

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

Great job and fixation 👍 Thank you.

Could you please add at least one test to confirm that the bug will not appear in the future?

@o100ja
Copy link
Author

o100ja commented Nov 18, 2021

Great job and fixation +1 Thank you.

Could you please add at least one test to confirm that the bug will not appear in the future?

Unfortunately I simply do not have the time to do so, especially considering that you already have somewhat of a test case:

exceljs/test/test-table.js

Line 54 in d786c4d

style: {font: {bold: true, name: 'Comic Sans MS'}},

The problem is that there isn't enough deep assertion on the finished xlsx file and that is not a trivial taks

@Siemienik Siemienik self-assigned this Apr 6, 2023
@jbuck-lineleap
Copy link
Contributor

jbuck-lineleap commented Jan 4, 2024

@Siemienik any chance of getting this merged? This is massively blocking me on a feature and i don't want to have to fork for this

@jbuck-lineleap jbuck-lineleap mentioned this pull request Jan 4, 2024
Merged
@jbuck-lineleap
Copy link
Contributor

jbuck-lineleap commented Jan 4, 2024

@Siemienik I've created an additional PR here #2649 with the same change, an update to the type declarations, and a test. Could you please merge this ASAP as it is blocking major functionality for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Siemienik Siemienik Siemienik left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Siemienik Siemienik

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@o100ja @jbuck-lineleap @Siemienik