Skip to content

fix: styles rendering in case when "numFmt" is present in conditional formatting rules (resolves #1814) #1815

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
Merged
merged 4 commits into from
Sep 7, 2021
Merged

fix: styles rendering in case when "numFmt" is present in conditional formatting rules (resolves #1814) #1815

merged 4 commits into from
Sep 7, 2021

Conversation

andreykrupskii
Copy link
Contributor

@andreykrupskii andreykrupskii commented Aug 16, 2021 •
edited
Loading

Summary

Closes #1814

This code covers fix for #1814. The issue is described by numFmt tag is rendered without numFmtId and formatCode properties when it is located in dxf tag (that is used for dynamic styles, for example, conditional formatting). This specific case is handled by the changes in the scope of this pull request.

Test plan

The next code snippet is suggested for testing:

const Excel = require('./excel');

const { WorkbookWriter } = Excel.stream.xlsx;

(async () => {
    const workbook = new WorkbookWriter({ filename: 'test.xlsx', useStyles: true });
    const worksheet = workbook.addWorksheet('example');

    const column = worksheet.getColumn(1);
    column.style.numFmt = '0.0#';

    worksheet.addConditionalFormatting({
        ref: 'A:A',
        rules: [{
            type: 'cellIs',
            operator: 'greaterThan',
            formulae: [1000],
            style: { numFmt: '0.0#,K' },
            priority: 1,
        }],
    });

    [100.5, 1000.5].forEach((value, index) => {
        const row = worksheet.getRow(index + 1);
        const cell = row.getCell(1);
        cell.value = value;
        row.commit();
    });

    worksheet.commit();
    await workbook.commit();
})();

Script execution result before fix apply:

corrupted-file

corrupted-xml

Script execution result after fix apply:

valid-file

valid-xml

Related to source code (for typings update)

Typings update is not required.

@andreykrupskii
Copy link
Contributor Author

@alubbe, @Siemienik, @Alanscut, I kindly ask you to review this pull request.

@Siemienik Siemienik self-requested a review August 17, 2021 06:35
@alubbe
Copy link
Member

alubbe commented Aug 17, 2021

thanks, this is a great PR! Could you please add a unit test for the xml generation?

@andreykrupskii
Copy link
Contributor Author

@alubbe, sure, I will add tests

@andreykrupskii andreykrupskii force-pushed the fix/issue-1814-conditional-numfmt branch from 596fa30 to e476df7 Compare August 20, 2021 11:12
@andreykrupskii
Copy link
Contributor Author

@alubbe, I covered this case with an integration test, thanks

@alubbe, @Siemienik, @Alanscut, I kindly ask you to review and merge it.

Siemienik
Siemienik approved these changes Aug 21, 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.

LGTM 🥇 Thank you for that contribution ;)

@andreykrupskii
Copy link
Contributor Author

@Siemienik, I double-checked tests locally and they passed, while I also was able to reproduce the problem when the pipeline got stuck due to test:jasmine script (looks like grunt-contrib-jasmine does not receive response from headless browser during setup). Please, advise, how can we proceed with this pull request?

@andreykrupskii
Copy link
Contributor Author

@alubbe, @Siemienik, @guyonroche, please, take a look at my previous comment and, please, advise.

@andreykrupskii andreykrupskii force-pushed the fix/issue-1814-conditional-numfmt branch from f7f3308 to 3e4d0a1 Compare September 5, 2021 11:28
@andreykrupskii
Copy link
Contributor Author

@Siemienik, @alubbe, @guyonroche, please, review one more time 😉

I've added a fix that prevents test:jasmine job from getting stuck - https://github.com/exceljs/exceljs/pull/1815/files#diff-e22a0e4b362f0115dd0b86fb682098a5eedfa816971c4c7932b8e8e106d3946dR125

The issue with stucking pipeline is described by grunt-contrib-jasmine library that embeds jasmine library inside and has an issue with running tests using the latest jasmine version. More info can be found via gruntjs/grunt-contrib-jasmine#339.

I've tested pipeline execution using my fork and it successfully passed - andreykrupskii#2

Feel free to ask any questions/suggestions and I look forward to the merge of this PR.

@alubbe
Copy link
Member

alubbe commented Sep 7, 2021

Thanks for this PR and unblocking jasmine!

@alubbe alubbe merged commit f150952 into exceljs:master Sep 7, 2021
@harunisik
Copy link

This issue is still happening in the latest version 4.3.0.

@andreykrupskii
Copy link
Contributor Author

Hi there @harunisik! I was just looking at the changes and I didn't see the latest update published. I noticed that V4.3.0 was published on Aug 21, 2021, and this change was merged on Sep 7, 2021. Hey @alubbe, do you happen to know the current status of this project and if it has active maintainers? Thanks so much!

@Siemienik
Copy link
Member

Hi, no one except Guyonroche has permission to publish package. Recently I publish it from fork https://github.com/SiemaTeam/spreadsheets .
Could you try if is it working properly from that fork?

btw. we're organizing MergeFest to cleanup current PRs list, they are on Discord https://discord.gg/siema .

@rpeight8
Copy link

Hi, no one except Guyonroche has permission to publish package. Recently I publish it from fork https://github.com/SiemaTeam/spreadsheets . Could you try if is it working properly from that fork?

btw. we're organizing MergeFest to cleanup current PRs list, they are on Discord https://discord.gg/siema .

I tried it today - so far so good. Thanks.

@j-mendez j-mendez mentioned this pull request Sep 8, 2024
Open
@Vishwas1 Vishwas1 mentioned this pull request Sep 8, 2024
Open
@j-mendez j-mendez mentioned this pull request Sep 13, 2024
Open
This was referenced Sep 15, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Schdoop Schdoop Schdoop approved these changes

@alubbe alubbe alubbe approved these changes

@Siemienik Siemienik Siemienik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Adding "numFmt" to conditional formatting rule corrupts document
6 participants
@andreykrupskii @alubbe @harunisik @Siemienik @rpeight8 @Schdoop